Skip to content

fix(session-flow): audit remediation (observer flag gate, export records, unattended parsing) - #5239

Merged
kyle-sexton merged 16 commits into
mainfrom
fix/audit-session-flow
Sep 29, 2026
Merged

kyle-sexton merged 16 commits into
mainfrom
fix/audit-session-flow

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

No related issue: audit remediation, the linked issues are Refs only

Summary

Remediates the session-flow findings of the audit of the unattended Cursor run (.work/audit/REPORT.md). Every linked issue is a Refs: none has all its criteria met on this branch. #3915 is ratified and stays closed. #4056 waits on the claude-ops close ritual. #4295 and #4284 are only partly addressed here. #4661 waits on the architecture and improvement description trims. #3952 and #3953 are owner decisions, so no option is implemented and the Park text stays. Releases session-flow 0.38.28.

Fix

  • Running-retro observer: --permission-prompts none is gated on Claude Code 2.1.259+ by the version helper in scripts/claude_cli.py, shared with hop_chain.py (Apply the Claude Code 2.1.257 to 2.1.263 changelog decisions: 20 corrections, 1 native nomination, 7 adoptions, 3 declines #4027).
  • keep-going: exit 1 of check-usage-limit-reset.py is provisional until a live re-check of the current account; with no live reading it asks the operator.
  • clean-stop, handoff and retro: the /export suggest sentence points at a real same-file record, and the self-ignore guard is created and announced when absent.
  • handoff, retro and clean-stop parse the leading unattended argument, with evals.
  • One record for the plugin data dir and plain-token substitution.
  • save_point.py memory-root is documented on the stale surfaces and allowed in the hop harness.
  • orchestrate records the hung-background-shell claim, handoff matches implement-dispatch, and the pending-CI record is linked.
  • Node.js on PATH is declared: a README Requirements section and a node probe in setup's check (every hook row launches through node hooks/exec-bash.mjs).
  • The 0.38.22 CHANGELOG entry no longer says the hook rows are unchanged, which contradicted 0.38.21 from the same commit.
  • setup's eval prompts carry the literal em dash instead of — escapes.
  • Version 0.38.28 and a CHANGELOG entry.

Verification

  • scripts/validate-plugins.sh: all manifests and the catalog validated.
  • scripts/check-changelog-parity.sh --check --check-order: pass.
  • pytest plugins/session-flow via uv run --with pytest: 243 passed, 6 subtests passed.
  • plugins/session-flow/scripts/save_point.test.sh and scripts/harness/hop_chain.test.sh: pass.
  • check-skill.sh over plugins/session-flow/skills: 14 passed, 0 failed.
  • check-listing-budget.sh: 6123/8000, no skill description changed.
  • check-changed-skills.sh origin/main: 7 checked, 0 failed.

Related

Refs: #3915
Refs: #3952
Refs: #3953
Refs: #4056
Refs: #4284
Refs: #4295
Refs: #4661

Findings in .work/audit/REPORT.md: #4027 (observer flag, owned by claude-config), #3915, #4056, #4295, #4661, #3952 and #3953 (owner decisions, reopened with decision packets).

Cross-group requests applied here: work-items (setup eval escapes) and hook-launcher (Node.js declaration, 0.38.22 wording). The tracker, improvement and decisions-docs requests on #4661 became one addendum comment; no owner decision was implemented.

Cross-group requests sent elsewhere:

🤖 Generated with Claude Code

https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB

kyle-sexton and others added 8 commits September 29, 2026 00:43
…erver

The observer appended --permission-prompts none unconditionally, while
hop_chain.py gated it on Claude Code 2.1.259 because older CLIs reject it
as an unknown option. Move the version probe and gate into
scripts/claude_cli.py and use it from both, so an older CLI runs the
analysis with dontAsk alone instead of failing.

Refs #4027

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
… adds a no-live-reading fallback

Exit 1 is provisional until a live re-check of the current account confirms it.
When no live reading is obtainable, ask the operator instead of concluding
still-blocked. The verification record quotes the fetched docs, drops
/rate-limit-options from the live sources, and the rule is stated once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
… guard creation

The /export suggest sentence in clean-stop, handoff, and retro pointed at a
verification record "in this section" that did not exist. Each skill now carries
a same-file four-part record (claim, basis, as-of date, recheck trigger) and the
sentence names its heading. The record states that /export has no documented
disable switch.

The sentence also dropped the create-and-announce behaviour for the memory
root's self-ignore guard; it is restored at all three sites. In retro the
sentence ended a paragraph with a stray "When the" and now stands alone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…retro and clean-stop

The three skills declared `unattended` in argument-hint but their parse
sections never consumed it, so handoff read it as the topic or a method and
retro read it as a mode. Each Arguments/mode section now strips it first and
scopes it to the /export step; evals cover both parse paths.

Refs #4056

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…n substitution

Add a four-part verification record for CLAUDE_PLUGIN_DATA in structure.md and
point find-handoff, topic-docs and save_point._plugin_data_root at it. The
plugins-reference page does not say Read-tool files or the ${VAR:-default} form
are substituted, so the running-retro arm block uses a plain substituted token
and the retro context files point at the score-history path SKILL.md states.

Refs #4295

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…s it left stale

Refs #4295

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…doff with dispatch, link the pending-CI record

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

…at /export is reserved for the person

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 29, 2026 05:15
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T05:18:50.420985Z 75a8dc5 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post findings as inline PR comments
  • Post summary comment

Scope note: .github/workflows/claude-security-review-hosted.yml exists in this repo, so per REVIEW.md this lane omits security findings — those belong to the security lane.

Note on verification: I read the diff and touched files directly but could not execute uv run pytest, scripts/run-ruff.sh, or the shell test harnesses in this sandbox (command execution is not permitted here). The PR body's Verification section (243 passed / check-changelog-parity.sh / check-skill.sh / etc.) is therefore author-claimed and unverified by me, not independently confirmed.

Findings

1 confirmed defect, posted inline:

  • plugins/session-flow/scripts/save_point.py:1315 — the _plugin_data_root() docstring ends with a stray, garbled sentence: ponytail: derivation assumes the cache layout above; a layout change gives None (refusal), recheck per the record. ponytail: isn't a label used anywhere else in this codebase and reads as an editing artifact rather than intended content. Suggested a fix inline (e.g. Caveat:).
    Permalink:
    ponytail: derivation assumes the cache layout above; a layout change gives None (refusal), recheck per the record."""

What I checked and found clean

  • claude_cli.py extraction: the --permission-prompts version gate is a verbatim move from hop_chain.py (no logic change); observer.py and hop_chain.py now both import it, and hop_chain.test.sh / test_observer.py were updated consistently to patch claude_cli._CLAUDE_VERSION_CACHE instead of the old hop_chain. attribute.
  • hop_chain.py READ_ONLY_SAVE_POINT_RE now also matches memory-root; verified the regex/is_write_indicator logic by inspection against the new hop_chain.test.sh cases (memory-root read-only, new still a write).
  • Doc cross-references: handoff/clean-stop/retro's new /export "Verification record" sections and the reference/pending-ci-caveat.md link resolve to real, pre-existing content; retro's score-history paths now point at SKILL.md "Paths" instead of hardcoding ${CLAUDE_PLUGIN_DATA}/scores/..., and that section still exists with matching content.
  • handoff's reworded "resident implement-dispatch" claim (clear / model switch / run end / user-only commit gate) matches the current plugins/implementation/skills/implement-dispatch/SKILL.md phase-boundary language and phase-boundary-owner.md's decision record — no drift.
  • running-retro's PLUGIN_DATA="${CLAUDE_PLUGIN_DATA}" indirection is consistent with this PR's own new "Verification record: plugin data dir" claim that substitution matches only the plain ${CLAUDE_PLUGIN_DATA} token, not a :-default form.
  • CHANGELOG/version bump (0.38.28) match the described fixes.

No design, error-handling, or test-coverage concerns beyond the one docstring defect above; this is otherwise a large, careful, documentation-and-refactor-heavy PR with correctness backed by inline evals for the new unattended-parsing behavior.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review complete

Ran /review:security-review (CI security lane) against PR #5239, HEAD 75a8dc5.

  • Gather context (diff, changed files)
  • Read changed files for security-relevant surface
  • Check for instruction-surface deletions against the exception register
  • Hunt for logic/trust-boundary/injection findings
  • Adversarially verify any candidate findings
  • Post inline comments for anchored findings / summary for cross-file findings
  • Final summary

No security issues found.

Scope covered:

  • scripts/claude_cli.py (new) — version-gate helper extracted from hop_chain.py, no behavior change, pure refactor (subprocess call unchanged: fixed argv list [claude, "--version"], no shell, no untrusted input).
  • scripts/harness/hop_chain.py — only the extraction above plus READ_ONLY_SAVE_POINT_RE widened to include memory-root. Verified save_point.py memory-root (cmd_memory_root, scripts/save_point.py#L1654-L1662) is genuinely read-only (resolves and prints a path, no writes), so classifying it as non-write in the harness's free-hand-bypass detector is correct, not a weakened check.
  • save_point.py — docstring/comment changes only (_plugin_data_root), no logic change.
  • skills/running-retro/scripts/observer.py — --permission-prompts none is now gated behind claude_version_at_least, matching hop_chain.py's existing gate; the permission mode (dontAsk) and read-only tool restriction (--tools Read, --strict-mcp-config) are unchanged, so the untrusted-observation boundary this script sits on is unaffected.
  • Skill bodies (clean-stop, handoff, retro, keep-going, orchestrate) — applied the instruction-surface-deletion lens. The unattended argument added to handoff/retro/clean-stop only gates whether the /export suggestion is voiced as a question vs. recorded in output; a new explicit line was added ("This skill never runs /export; it is reserved for the person to run") tightening rather than loosening that control. keep-going's exit-1 handling changed from an unconditional hand-back to "provisional until a live re-check" — also a tightening, not a relaxation of a guardrail. No removed rule was found that isn't still enforced elsewhere (redaction gate in clean-stop step 3, the unattended remote-artifact behavior it documents, is pre-existing and unchanged by this PR).
  • Remaining files (reference docs, evals, CHANGELOG, plugin.json) are prose/metadata only, no executable or trust-boundary surface.

No GitHub Actions workflow files are touched by this PR.

@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: 75a8dc582e

ℹ️ 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/session-flow/scripts/harness/hop_chain.py
Comment thread plugins/session-flow/skills/retro/SKILL.md Outdated
Comment thread plugins/session-flow/scripts/save_point.py Outdated
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post findings as inline PR comments
  • Post summary comment

Scope note: a dedicated security lane (/review:security-review) already ran on this PR, so per the lane's boundary this review omits security findings.

Reviewed: current HEAD ff86882 (base origin/main), diff unchanged from the prior review's 75a8dc5 except one new commit (1fbf3ad, docs-only: Requirements section, setup Node.js probe, changelog wording, eval em-dash fix — no issues found there).

Note on verification: I read the diff and touched files directly but did not execute pytest, the shell test harnesses, or the lint/validation scripts in this pass. The PR body's Verification section (243 passed, check-changelog-parity.sh, check-skill.sh, etc.) is therefore author-claimed and unverified by me, not independently confirmed.

Findings (2 issues, 4 locations) — posted inline

Both were already flagged by Codex's review on this PR and remain unresolved on this HEAD (the touched lines are unchanged since):

  1. save_point.py:1315 — a stray ponytail: label in the _plugin_data_root() docstring, an apparent editing artifact inconsistent with this PR's own Claim./Basis./Recheck when. verification-record style.
  2. handoff/SKILL.md:330, clean-stop/SKILL.md:61, retro/SKILL.md:131 — the same self-contradictory /export sentence ("run it for a durable conversation copy" immediately followed by "This skill never runs /export; it is reserved for the person to run") is now in all three skills, not just retro where Codex originally flagged it. Each file's own new Verification record: /export section states the command is unavailable headless/via the Skill tool, so the "run it" clause is directly contradicted by the record two lines below it. Codex's ask to make the three skills consistent has technically been satisfied — but by propagating the contradiction to all three rather than resolving it.

What I checked and found clean

  • scripts/claude_cli.py extraction / hop_chain.py's use of it — verbatim move, no logic change, consistent with the prior review pass.
  • running-retro/scripts/observer.py's permission_prompts_args(claude) gate and its new test_observer.py coverage (None and pre-floor version cases).
  • hop_chain.py's READ_ONLY_SAVE_POINT_RE widened to include memory-root — save_point.py memory-root is genuinely read-only (prints a path).
  • keep-going/SKILL.md's reworded exit-1 handling (provisional until live re-check, operator fallback when no live reading is obtainable) and its eval/expectations update.
  • handoff/retro/clean-stop's new leading unattended argument parsing and the doc cross-references (reference/pending-ci-caveat.md, reference/structure.md "Verification record: plugin data dir", find-handoff's save_point.py memory-root fallback wording) all resolve to real, matching content.
  • orchestrate/context/sources.md's new hung-background-shell record is properly labeled as a single local observation, not an upstream-documented behavior.
  • CHANGELOG/version bump (0.38.28) match the described fixes.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review (/review:security-review)

  • Gather context (PR diff, changed files, custom instructions)
  • Invoke /review:security-review and follow its instructions exactly
  • Post findings

No security issues found.

Scope: full diff origin/main...HEAD (31 files, all under plugins/session-flow/), per the skill's criteria (logic/trust-boundary/injection findings; GitHub Actions hardening deferred to zizmor; instruction-surface deletions checked against the exception register).

Code changes since the last complete security review on this PR (commit 75a8dc5, "No security issues found"). git diff 75a8dc5..HEAD -- plugins/session-flow shows only CHANGELOG.md, README.md, and setup/SKILL.md/evals.json — declaring the Node.js prerequisite and fixing an em-dash escape and a changelog wording claim. No executable or trust-boundary surface changed in that range, so the prior review's verdict still covers the current head SHA (ff86882f9329f0d0b04a3252dd5ae6d3cf7187a9).

Full-PR review (against origin/main), confirming the prior verdict:

  • scripts/claude_cli.py (new): pure extraction from hop_chain.py, fixed-argv subprocess.run([claude, "--version"], ...), no shell=True, no untrusted input.
  • scripts/harness/hop_chain.py: only the extraction above plus READ_ONLY_SAVE_POINT_RE widened to include memory-root. A reviewer (Codex, inline on this PR) flagged that is_write_indicator() short-circuits to non-write on any save_point.py ... memory-root substring, so a compound shell command pairing it with a second, shell-invisible write could slip past skill_evidence()'s free-hand-bypass detector at hop_chain.py:617. That match-anywhere-in-the-command design is pre-existing for validate/emit (unchanged by this diff, not introduced here), and its only consumer gates a dev/CI test-harness assertion against a live model in a throwaway fixture repo already run under bypassPermissions — not a production trust boundary over real user data or secrets. I agree it's a genuine, worth-fixing gap, but it doesn't clear this lane's bar as a new exploitable path introduced by this diff, so I'm not filing it as a separate finding, just corroborating Codex's existing comment.
  • skills/running-retro/scripts/observer.py: --permission-prompts none now gated behind claude_version_at_least; --permission-mode dontAsk and the read-only --tools Read / --strict-mcp-config restriction over the untrusted-observation boundary are unchanged.
  • Skill bodies (clean-stop, handoff, retro, keep-going, orchestrate, setup, running-retro): checked under the instruction-surface-deletion lens. The unattended argument only changes whether the /export suggestion is voiced as a question or recorded in output; each skill also gained an explicit new line ("This skill never runs /export; it is reserved for the person to run") tightening, not loosening, that boundary. keep-going's exit-1 handling changed from an unconditional hand-back to "provisional until a live re-check" — also a tightening. No removed rule found that isn't still enforced elsewhere.
  • Remaining files (reference docs, evals, CHANGELOG, plugin.json version bump) are prose/metadata only.

No GitHub Actions workflow files are touched by this PR.

Comment thread plugins/session-flow/scripts/save_point.py Outdated
Comment thread plugins/session-flow/skills/handoff/SKILL.md Outdated
Comment thread plugins/session-flow/skills/clean-stop/SKILL.md Outdated
Comment thread plugins/session-flow/skills/retro/SKILL.md Outdated
kyle-sexton and others added 2 commits September 29, 2026 09:51
…ument-hint rewrite

#5042 shortened the argument-hint key in skills/interview/SKILL.md and moved
the examples to the body. Neither defense pinned by interview-defenses.test.sh
is touched by that change, so the whole-frontmatter digest is updated. The
suite was failing on main for every pull request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
kyle-sexton added a commit that referenced this pull request Sep 29, 2026
…rview frontmatter digest

- setup check: Node.js stays FAIL when the toggle is off (the runner starts node
  before the gate runs) and a Bash PASS is scoped to Bash's PATH; the probe's walk
  limit and file tests are read from probe-prerequisite.sh instead of restated.
- planning: re-pin the interview SKILL.md frontmatter digest. #5042 shortened the
  argument-hint key only (the two defenses are body prose, digested separately), and
  the stale digest fails test-linux on main. Same one-line change as #5239.

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

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post findings as inline PR comments
  • Post summary comment

Scope note: a dedicated security lane (/review:security-review) already ran on this PR, so per the lane's boundary this review omits security findings.

Reviewed: HEAD c8ee7dd (base origin/main). The session-flow content is unchanged since the last full code-review pass on this PR (ff86882): git diff ff86882..c8ee7dd touches only unrelated files pulled in by merging origin/main (plugins/ai-slop/**, plugins/evals/**, plugin.json version bumps) plus one line in plugins/planning/tests/interview-defenses.test.sh re-pinning a frontmatter digest for an unrelated main-branch fix (commit 5a077a8, rationale given in its message: main's #5042 argument-hint rewrite broke this suite's pin for every PR). I did not execute interview-defenses.test.sh to independently confirm the new digest (shell-script execution isn't available to me here); it reads as a legitimate carry-forward fix, not part of this PR's session-flow scope.

Note on verification: I read the diff and touched files directly but did not execute pytest, validate-plugins.sh, check-changelog-parity.sh, or the shell test harnesses in this pass. The PR body's Verification section is therefore author-claimed and unverified by me, not independently confirmed.

Findings (2 issues, 4 locations) — posted inline

Both were already raised by Codex and prior Claude review passes on this PR and remain unresolved on this HEAD (touched lines unchanged since ff86882):

  1. save_point.py:1315 — a stray ponytail: label in the _plugin_data_root() docstring, inconsistent with this PR's own Claim./Basis./Recheck when. verification-record style.
  2. handoff/SKILL.md:330, clean-stop/SKILL.md:61, retro/SKILL.md:131 — the same self-contradictory /export sentence ("run it for a durable conversation copy" immediately followed by "This skill never runs /export; it is reserved for the person to run") is present in all three skills. Each file's new Verification record: /export section clarifies the intent in prose, but the checklist line itself still reads as a direct imperative immediately negated in the same sentence — worth tightening before merge across all three for consistency, as Codex originally requested.

What I checked and found clean

  • scripts/claude_cli.py extraction / hop_chain.py's use of it, and the READ_ONLY_SAVE_POINT_RE widening to include memory-root — consistent with prior passes; the harness free-hand-bypass scoping concern Codex raised on this line was already evaluated by the security lane and judged not a new exploitable path (CI-only fixture harness), so I'm not re-raising it here as a code-review finding.
  • running-retro/scripts/observer.py's version-gated --permission-prompts none and its test_observer.py coverage.
  • keep-going/SKILL.md's reworded exit-1 handling and eval updates.
  • handoff/retro/clean-stop's new leading unattended argument parsing and doc cross-references (reference/pending-ci-caveat.md, reference/structure.md, find-handoff's memory-root fallback wording).
  • orchestrate/context/sources.md's new hung-background-shell record, correctly labeled as a single local observation.
  • README/CHANGELOG Node.js declaration, setup's node probe, and the 0.38.28 version bump.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review (/review:security-review)

  • Gather context (diff, changed files vs origin/main)
  • Invoke /review:security-review and follow its instructions exactly
  • Post findings

No security issues found.

Skip-gate check (criterion 3, "this head already has a successful security review that still applies"): git diff ff86882..c8ee7dd (the commit reviewed and cleared in the prior complete security-review pass on this PR) touches only files outside plugins/session-flow — .claude/ai-slop.json, plugins/ai-slop/**, plugins/evals/** — all incoming from merging origin/main, none of which are part of this PR's actual diff against origin/main (the merge base absorbs them). The one file from that range that is part of the PR diff, plugins/planning/tests/interview-defenses.test.sh, is a one-line SHA-digest pin update (re-pinning a frontmatter hash after an unrelated skill's argument-hint rewrite) — no security-relevant surface, no logic change.

So the full origin/main...HEAD diff at this head (c8ee7dde21ef5a10662a9828c54364930de409c5, 32 files, all under plugins/session-flow/ plus the one test-hash line above) is identical in security-relevant content to what was already reviewed and cleared at commit ff86882 ("No security issues found," corroborating the earlier 75a8dc5 pass). Re-confirming that verdict:

  • scripts/claude_cli.py (new): pure extraction from hop_chain.py, fixed-argv subprocess.run([claude, "--version"], ...), no shell=True, no untrusted input.
  • scripts/harness/hop_chain.py: extraction above plus READ_ONLY_SAVE_POINT_RE widened to include memory-root. Codex's inline finding (substring-anywhere match lets a compound shell command pair the read-only lookup with a shell-invisible second write, slipping past skill_evidence()'s free-hand-bypass detector) is a real, worth-fixing gap in a pre-existing match-anywhere design (validate/emit already had it, unchanged by this diff) whose only consumer gates a dev/CI harness assertion in a throwaway fixture repo run under bypassPermissions — not a production trust boundary. Not a new exploitable path introduced by this diff, so not filed as a separate finding here, consistent with the prior pass.
  • skills/running-retro/scripts/observer.py: --permission-prompts none now gated behind claude_version_at_least; --permission-mode dontAsk and the read-only --tools Read / --strict-mcp-config restriction over the untrusted-observation boundary are unchanged.
  • Skill bodies (clean-stop, handoff, retro, keep-going, orchestrate, setup, running-retro) checked under the instruction-surface-deletion lens: the unattended argument only changes whether the /export suggestion is voiced as a question or recorded in output; each skill also gained an explicit new line ("This skill never runs /export; it is reserved for the person to run") tightening, not loosening, that boundary. keep-going's exit-1 handling changed from an unconditional hand-back to "provisional until a live re-check" — also a tightening. No removed rule found that isn't still enforced elsewhere.
  • Remaining files (reference docs, evals, CHANGELOG, plugin.json version bump) are prose/metadata only.

No GitHub Actions workflow files are touched by this PR.

Comment thread plugins/session-flow/scripts/save_point.py Outdated
Comment thread plugins/session-flow/skills/handoff/SKILL.md Outdated
Comment thread plugins/session-flow/skills/clean-stop/SKILL.md Outdated
Comment thread plugins/session-flow/skills/retro/SKILL.md Outdated
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Check the lane's skip gate
  • Post summary comment

Skip gate applies (criterion 4): this head already has a successful, still-applicable review from this lane.

HEAD is 5b6eaf8 (fix/audit-session-flow branch). The prior complete code-review pass on this PR ran at c8ee7dd (2026-09-29T13:53Z). git diff c8ee7dd..5b6eaf8 touches only:

plugins/planning/.claude-plugin/plugin.json | 2 +-
plugins/planning/CHANGELOG.md               | 6 ++++++

— a version bump and changelog entry for an unrelated plugin (planning 0.45.13, releasing the interview-defenses.test.sh digest re-pin that was already reviewed and found clean at the prior pass). Nothing under plugins/session-flow/ — this PR's actual scope — changed in that range, so the c8ee7dd review's findings and clean-scope verdict still apply unchanged at 5b6eaf8.

Findings from the c8ee7dd pass remain open (unresolved across three prior review passes at 75a8dc5, ff86882, and c8ee7dd; touched lines unchanged since):

  1. save_point.py:1315 — stray ponytail: label in the _plugin_data_root() docstring, inconsistent with this PR's own Claim./Basis./Recheck when. verification-record style.
  2. handoff/SKILL.md:330, clean-stop/SKILL.md:61, retro/SKILL.md:131 — the same self-contradictory /export sentence ("run it for a durable conversation copy" immediately followed by "This skill never runs /export; it is reserved for the person to run"), directly contradicted by each file's own new Verification record: /export section stating the command is unavailable headless/via the Skill tool. Originally flagged by Codex on the retro copy; now present in all three skills.

Per the skip gate, this pass posts no new inline comments (they'd duplicate the existing ones already on this PR at 75a8dc5/ff86882/c8ee7dd) — both findings are still visible above for tracking. Note the prior pass's own caveat still holds: the PR body's Verification section (pytest, check-changelog-parity.sh, check-skill.sh, etc.) is author-claimed and not independently re-confirmed by this pass.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review (/review:security-review)

  • Gather context (PR diff, changed files vs origin/main)
  • Invoke /review:security-review and follow its instructions exactly
  • Post findings

No security issues found.

Skip-gate check (criterion 3, "this head already has a successful security review that still applies"): git diff c8ee7dd..5b6eaf8 touches only plugins/planning/.claude-plugin/plugin.json (version bump 0.45.12 → 0.45.13) and plugins/planning/CHANGELOG.md (matching changelog prose) — a version-bump/changelog commit released for the interview-digest re-pin already reviewed on c8ee7dd. No plugins/session-flow file changed in this range, and no other file in the full origin/main...HEAD diff changed either. So the current head (5b6eaf8a999ab166392b8bc00568bdc31168dbd5, 34 files, 315+/132-) is security-relevant-identical to what was already reviewed and cleared at commit c8ee7dd ("No security issues found," itself corroborating ff86882 and 75a8dc5).

Re-confirming that verdict, which still applies at this head:

  • scripts/claude_cli.py (new): pure extraction from hop_chain.py, fixed-argv subprocess.run([claude, "--version"], ...), no shell=True, no untrusted input.
  • scripts/harness/hop_chain.py: extraction above plus READ_ONLY_SAVE_POINT_RE widened to include memory-root. Codex's inline finding (substring-anywhere match lets a compound shell command pair the read-only lookup with a shell-invisible second write, slipping past skill_evidence()'s free-hand-bypass detector) is a real, worth-fixing gap in a pre-existing match-anywhere design (validate/emit already had it, unchanged by this diff) whose only consumer gates a dev/CI harness assertion in a throwaway fixture repo run under bypassPermissions — not a production trust boundary. Not a new exploitable path introduced by this diff, so not filed as a separate finding here, consistent with every prior pass on this PR.
  • skills/running-retro/scripts/observer.py: --permission-prompts none now gated behind claude_version_at_least; --permission-mode dontAsk and the read-only --tools Read / --strict-mcp-config restriction over the untrusted-observation boundary are unchanged.
  • Skill bodies (clean-stop, handoff, retro, keep-going, orchestrate, setup, running-retro) checked under the instruction-surface-deletion lens: the unattended argument only changes whether the /export suggestion is voiced as a question or recorded in output; each skill also gained an explicit new line ("This skill never runs /export; it is reserved for the person to run") tightening, not loosening, that boundary. keep-going's exit-1 handling changed from an unconditional hand-back to "provisional until a live re-check" — also a tightening. No removed rule found that isn't still enforced elsewhere.
  • Remaining files (reference docs, evals, CHANGELOG, plugin.json version bumps in both session-flow and planning, and the one-line interview-defenses.test.sh digest re-pin) are prose/metadata/test-fixture only.

No GitHub Actions workflow files are touched by this PR.

kyle-sexton added a commit that referenced this pull request Sep 29, 2026
…ind notice to manifest (#5284)

Refs: #4240

## Summary

Audit findings against `plugins/biome-format` for #4240 (missing-tool
prerequisites). The SessionStart probe ignored the
`biome_format_enabled` kill switch, the hook's notice text was not bound
to `prerequisites.json`, and the README, setup skill and check skill
disagreed on notice wording and left out Node and the probe. Coverage of
the other five binary-probing plugins and the two owner questions on
#4240 stay open, so this PR only refs the issue.

## Fix

- `hooks/hooks.json`: the SessionStart row launches with
`--run-if-unset-or-true BIOME_FORMAT_ENABLED` (the form typos-format
uses). `probe-prerequisite.sh` and `exec-bash.mjs` are untouched; the
probe stays byte-identical across biome-format, go-format and
markdown-format.
- `hooks/biome-format.test.sh`: runs the row as the harness spawns it,
from an empty cwd on a PATH without biome. Disabled prints nothing;
unset and true print the notice. A missing node fails the suite. A new
case binds the hook's missing-binary notice (tool name, check, install
line) and the tool's local bin path to `prerequisites.json`.
- README, `setup` and `check` skills, and their evals: the two latch
classes are stated once, Node.js and the probe are documented, the setup
check names the probe as a separate resolver, adds a Node row and
reports `biome_format_lint_gitignored`, and the check skill runs every
probe through Bash because setup's pre-computed rows are not rendered
when it reads the file.
- Release 0.7.6 with a CHANGELOG entry.
- `plugins/planning/tests/interview-defenses.test.sh`: re-pins the
interview `SKILL.md` frontmatter digest that #5042 (argument-hint only)
left stale and that failed `test-linux` on main; planning 0.45.13. Same
one-line change as #5239.
- `CHANGELOG.md`, in-place corrections to released entries (each also
named in the 0.7.6 entry): 0.7.2 said this plugin's hook rows were
unchanged although 0.7.1, the same commit, changed them; 0.6.59 and
0.6.58 described `hook::shell_c_operand` and
`hook::bash_parse_segments`, which no hook here calls. All three now
read "shared launcher/library sync; no change to this plugin's behavior"
with their issue links kept. 0.7.3 stays: `hook::begin` calls
`hook::repo_relative_path_to`. No entry is renumbered or folded.
- `docs/formatter-path-probes.md`: the dispatch-completeness block is a
pointer at #3549, and the "do not raise `PostToolUse` timeouts" guidance
is bound to the recheck (the Windows `claude --debug` result on #3549)
instead of standing as fleet guidance. The four-part record stays in
this file.

Not done, owner-reserved: whether the probe may notify in repos with no
Biome config (Q1) and whether to keep the per-plugin check skills (Q2).
No option is implemented.

## Verification

- `bash plugins/biome-format/hooks/biome-format.test.sh`: PASS=62 FAIL=0
- `bash scripts/check-prerequisite-probes.test.sh`: 18 cases, 0 failed
- `bash scripts/check-changelog-parity.sh --check --check-order`: pass
- `bash scripts/validate-plugins.sh`: all manifests and the catalog
validated
- `check-cross-plugin-source-drift.sh`, `check-hook-exec-form.sh`,
`check-killswitch-hoist.sh`, `check-skill-leaf-names.sh`,
`check-purged-em-dashes.sh`, `check-silent-skips.sh`: no findings for
this plugin
- Mutation checks (task runs): reverting the hooks.json flag fails the
disabled case; changing the install string in the hook only fails the
binding case.

## Related

- Audit report `.work/audit/REPORT.md`: row 3b #4240 (partial delivery);
plugin-biome-format findings carried by hook-launcher (probe ignores
toggle, setup check lacks node and gitignore option).
- Cross-group requests: markdown-format and go-format gate their
SessionStart rows the same way; bash-format, ruff-format, typos-format,
actionlint and powershell-format correct their 0.x.5 CHANGELOG claims;
core-docs and conventions hold the check-skill split doc edits until Q2
is answered; scripts fixes hook-utils sync bumps; hook-launcher owns
`exec-bash.mjs`.
- Cross-group requests received and handled: hook-launcher F13, F37 and
F38 (kill switch, Node and gitignore option in setup check, and the
CHANGELOG rewording above; the `biome.json` opt-in gate on the probe is
owner question Q1 on #4240 and is not implemented); markdown-format (its
probe is in the Q1 packet on #4240, option C, and stays unchanged);
context7 (noted on #4240 with draft PR #5252, plus the playwright gap);
conventions (the two `docs/formatter-path-probes.md` edits above, with
#3549 as the recheck carrier). The conventions request for the
check-skill invocation-mode doc is held with Q2.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 2 commits September 29, 2026 10:42
…ng label and harness exemption

- clean-stop, handoff and retro: the /export checklist line now asks the person to run it
  instead of contradicting itself.
- save_point.py: replace the stray `ponytail:` label with `Caveat:`.
- hop_chain.py: judge the read-only save_point.py exemption per command segment, so a chained
  second interpreter call still counts as a write; two contract tests added.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
Take origin/main's planning release 0.45.13, which already carries the interview digest re-pin.

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

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review (/review:security-review)

  • Gather context (PR diff, changed files vs origin/main)
  • Check skip gate (prior review at this head?)
  • Review the delta since the last reviewed commit
  • Post findings

No security issues found.

Skip-gate check (criterion 3): the last complete security-review pass on this PR covered commit 5b6eaf8 ("No security issues found," itself corroborating three earlier passes at 75a8dc5, ff86882, c8ee7dd). HEAD is now c3002ba (this PR's --permission-prompts custom-instructions HEAD SHA), which contains new commits since 5b6eaf8, so the skip gate does not apply as-is; I reviewed the delta.

git diff 5b6eaf8..c3002ba touches many files outside plugins/session-flow (retro-audio, ruff-format, typos-format, visualization, …) — all merge noise from chore: merge origin/main into fix/audit-session-flow, not part of this PR's actual changes (this PR's file list stays scoped to plugins/session-flow/). The plugins/session-flow portion of that delta is the fix commit (729bf06) addressing the three review threads open on this PR:

  1. scripts/harness/hop_chain.py — is_write_indicator now judges per command segment. This closes Codex's finding on this PR (a compound command like save_point.py memory-root && mutate.py previously matched READ_ONLY_SAVE_POINT_RE against the whole string and returned "not a write," silently exempting the second, genuinely mutating call). The new COMMAND_SEPARATOR_RE.split(command) classifies each segment independently: WRITE_INDICATOR_RE/WRITE_CMDLET_RE still run against the full command first (so redirects/mutating verbs anywhere still short-circuit to "write"), and now each segment must independently clear READ_ONLY_SAVE_POINT_RE to avoid being flagged. I traced this against the two new contract tests in hop_chain.test.sh (memory-root && mutate.py, and validate x\nmutate.py via a literal newline) — both correctly return True now. As previously assessed on this PR, this detector's only consumer gates a dev/CI harness assertion in a throwaway fixture repo run under bypassPermissions, not a production trust boundary, but the fix is a genuine, correctly-scoped closure of the gap regardless.
  2. scripts/save_point.py:1315 — the stray ponytail: docstring label is now Caveat:. Cosmetic, no security surface.
  3. clean-stop/handoff/retro SKILL.md — the /export checklist line changed from a self-contradictory "run it ... This skill never runs /export; it is reserved for the person to run" to "suggest that the person run it ... This skill never invokes /export itself." Checked under the instruction-surface lens: this is a clarification that tightens the existing "skill never runs it itself" boundary into an unambiguous single reading, not a removal or loosening of any control.

No GitHub Actions workflow files are touched by this PR.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review)

  • Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post findings as inline PR comments
  • Post summary comment

Reviewed: HEAD c3002ba (branch fix/audit-session-flow). The prior complete code-review pass on this PR ran at 5b6eaf8 (2026-09-29T14:07Z), which found no open code issues in scope (only an unrelated planning version bump had landed since c8ee7dd, whose findings were the three items below).

New content since 5b6eaf8: commit 729bf06 ("address review threads on the export line, docstring label and harness exemption"), followed by a merge of origin/main (c3002ba) that pulled in unrelated plugins (discovery, firecrawl, machine-health, overengineering, plugin-quality, retro-audio, typos-format, etc. — all outside this PR's actual diff, per the PR's own 31-file changed-files list, which is entirely under plugins/session-flow/).

729bf06 is the fix commit for all three findings raised by Codex and prior Claude passes at 75a8dc5/ff86882/c8ee7dd/5b6eaf8. I re-verified each fix directly against the current file content:

  1. save_point.py:1315 — the stray ponytail: label is now Caveat:. Resolved.
  2. handoff/SKILL.md:330, clean-stop/SKILL.md:61, retro/SKILL.md:131 — the self-contradictory /export sentence is reworded to "suggest that the person run it ... This skill never invokes /export itself" in all three files, consistently. Resolved.
  3. hop_chain.py:117-126 — Codex's P2 finding (the read-only save_point.py exemption matched anywhere in the command, so a chained second interpreter call like save_point.py memory-root && python mutate.py slipped past the write detector). is_write_indicator() now splits the command on [;|&\n]+ and judges the read-only exemption per segment, so any other segment that invokes an interpreter still counts as a write. I traced the regex logic by hand against both new test cases in hop_chain.test.sh (... memory-root && python3 /p/mutate.py and a newline-separated validate + mutate.py) and confirmed both now correctly classify as writes. Resolved, with test coverage added.

No new findings. I did not execute pytest or the shell test harnesses in this pass (not available here); the fix's own contract tests read as correct by inspection, consistent with the author's claim in their inline reply on this thread.

@kyle-sexton
kyle-sexton merged commit 22ca621 into main Sep 29, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the fix/audit-session-flow branch September 29, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant