Skip to content

fix(session-flow): root the resume-prompt path and accept both forms on recovery - #1780

Merged
kyle-sexton merged 8 commits into
mainfrom
fix/1644-rooted-resume-prompt-path
Jul 30, 2026
Merged

fix(session-flow): root the resume-prompt path and accept both forms on recovery#1780
kyle-sexton merged 8 commits into
mainfrom
fix/1644-rooted-resume-prompt-path

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

The defect

/session-flow:handoff wrote the save-point to an absolute location but emitted a rootless
path in the copy-paste resume prompt. This was contract-specified, not a model slip:
reference/save-point.md defined the directive as
Read @<handoffs-dir>/<TS>-handoff-<topic>.md where <handoffs-dir> is "the path the write step
actually used" — and <memory_dir> is repo-relative by contract, so the one artifact an operator
carries across /clear lost the root the file hangs off.

Pasted into a session whose cwd is not the worked-in repository root, the @-reference resolves
somewhere else. When that somewhere else has its own .work/handoffs/ — true of any home directory
that has run /handoff before — the failure presents as "the file is missing" rather than "the
path has no root"
, which #1644 correctly calls the most expensive shape to diagnose. It is not only
a cross-repo problem: a resuming session sitting in a subdirectory of the right repo fails the
same way.

/session-flow:find-handoff, the skill that exists to recover exactly this, carried the same
single-root assumption — its transcript rung found the correct directive, resolved it against the
source transcript's cwd, and then discarded the candidate on the existence check.

The change

Producer (reference/save-point.md).

  • The directive now carries the absolute, forward-slash-normalized path. Forward slashes are
    specified rather than left to the model: the directive survives into transcript JSONL, where a
    backslash is escaped again, and find-handoff greps that record.
  • A Handoff origin: line inside the rails names the repository identity and the repo-relative
    path, so a resume on a different machine or checkout can re-resolve. It is computed at emit
    time
    from the repository actually written into — deliberately not a stored frontmatter field,
    which is what kept this change out of schema territory. It sits inside the rails because the copy
    region is what travels; below the rail it would be lost on paste.
  • The @ mention is documented as an accelerator, not the mechanism. Official docs state an
    @ reference's path "can be relative or absolute"
    (common-workflows),
    but document no drive-letter or whitespace-bearing form — so expansion is treated as unverified
    there, and the directive is written to stay actionable without it (the same line states the full
    absolute path, which a resuming session reads directly). That is what makes rooting a strict
    improvement rather than a trade.

Absolute is not new to this engine: reference/topic-docs.md already lands no-project-root handoffs
under ${CLAUDE_PLUGIN_DATA}/topic-docs/handoffs/ "with the absolute path announced prominently".
Same condition, reached a different way.

Consumer (skills/find-handoff/SKILL.md).

  • The detection contract accepts both forms. Every handoff written before this shipped states a
    repo-relative path and is still on disk, so a detector that recognizes only rooted directives
    would stop recovering the entire existing corpus. Matching happens on the
    …handoffs/<TS>-handoff-… shape both forms share; they diverge only at the existence check.
  • A path that resolves to nothing is UNRESOLVED, never discarded — on BOTH forms. A rootless
    miss because resolving against the producer's cwd is an inference; a rooted miss because an
    absolute path is machine-local and a resume on another machine or checkout cannot satisfy it. The
    rooted miss is exactly what Handoff origin: exists for, so the existence check reads that line
    and re-resolves against the repository it names before giving up. Either way: one bounded,
    read-only widening over repository roots already in hand, then surfaced at the confirm gate with
    the directive verbatim — and the gate names which failure it was, because "the path has no
    root" and "nothing is at that absolute path on this machine" send an operator to different places,
    while "missing" sends them nowhere. Discarding on miss is the specific behavior that made the
    recovery ladder unable to recover the failure it was written for.
  • Handoff origin: is a resolution input, not a detection signal. It cannot admit or reject a
    candidate, so it is not a fourth key — it is read only after a candidate qualifies, at the
    existence check. The signal summary says so rather than listing three signals while the ladder
    depends on a fourth thing.
  • The Gotcha keeping rootless resolution alive is preserved, not replaced.

An independent fresh-context review of this diff caught the rooted-miss hole: the first revision
consulted Handoff origin: only on the rootless branch — where the contract says it can never
appear, since the line shipped with the rooted form — leaving the one case it exists for as the one
case nothing handled, which fell through to discard-on-miss and reintroduced the defect one path
over. That, and the signal-label collision with the /loop re-arm note's existing "fourth signal",
are fixed in the second commit.

What this PR deliberately does NOT do

It does not fully satisfy #1644's line 176 ("rung 1's missing repo-correlation check should be
closed in the same change"). Closing it needs durable repository identity stored in the handoff
file
— a new frontmatter field — which is a cross-cutting schema change every handoff already on
disk would lack, and which every consumer must then tolerate the absence of. That is a decision on
its own merits, now filed as #1778 with the options laid out.

A weaker substitute (read the repository off the producer transcript) was considered and rejected on
evidence: it depends on a transcript that may be absent — while find-handoff's own Gotchas say
transcripts are the reliable index precisely because the filesystem is not — and it returns nothing
for every rootless legacy handoff, i.e. exactly where the check is needed. Shipping it would have
produced a check that looks closed and is not.

Instead, rung 1 now states the gap in place, so the next reader does not mistake it for closed.

Verification

All run locally in the PR worktree against origin/main:

  • scripts/check-changelog-parity.sh --check — every versioned plugin has a CHANGELOG.md
  • scripts/check-changelog-parity.sh --check-bump origin/main — 0.17.19 → 0.17.20 has its
    ## [0.17.20] entry, newly added
  • scripts/check-changelog-parity.sh --check-order — all 71 changelogs newest-first, no duplicates
  • scripts/check-changed-skills.sh origin/main — 2 skills checked, 0 failed (find-handoff carries
    the pre-existing >200-line soft warning; cap is 500, it is at 394)
  • scripts/check-skill-portability.sh origin/main — no unexcused coupling tokens
  • scripts/validate-plugins.sh — all plugin manifests + catalog valid
  • scripts/check-orphaned-fixtures.sh --check, check-contract-slice-prune.sh --check-diff origin/main, check-cross-plugin-source-drift.sh --check — all pass
  • markdownlint-cli2 "plugins/session-flow/**/*.md" — 36 files, 0 errors
  • session-flow's own contract tests (parse-transcript.test.sh, observer.test.sh) — PASS
  • Both touched evals.json files validate against plugins/skill-quality/reference/evals.schema.json.
    Eval coverage added for the producer emitting a rooted path when cwd is not the worked-in repo
    (handoff ci: onboard to the CI platform (ci-workflows + standards) #4), both-forms acceptance (find-handoff chore: adopt widened comment-hygiene policy and re-pin ci-workflows @9567b8b #9), the rootless UNRESOLVED path (build(deps): bump ci-workflows pin to b6431a1 (PSSA RULE_ERROR retry fix) #10, with its
    prompt pinned to the pre-rooted directive so the branch is unambiguous), and the rooted miss
    re-resolved via Handoff origin: on another machine (fix(markdown-formatter): harden hook + simplify plumbing #11). find-handoff eval chore: initialize marketplace scaffold #1's cwd-resolution
    expectation was made form-aware so it no longer asserts the old single-form behavior.

The harness claim this change rests on was verified against current official docs this session
rather than recalled, per the repo's fresh-docs mandate; the drive-letter and whitespace edge is
explicitly flagged as not covered by those docs and is handled by not relying on expansion.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C

kyle-sexton and others added 2 commits July 30, 2026 00:34
…on recovery

The save-point engine specified the resume directive as
`Read @<memory_dir>/handoffs/<TS>-handoff-<topic>.md`, and `memory_dir` is
repo-relative by contract — so the one artifact an operator carries across
`/clear` lost the root the handoff file was written under. Pasted into a session
whose cwd is not the worked-in repository root, the reference resolves somewhere
else; when that somewhere else has its own handoffs directory, the failure reads
as "the file is missing" rather than "the path has no root".

The directive now carries the absolute, forward-slash-normalized path, and a
`Handoff origin:` line inside the rails names the repository and repo-relative
path so a resume on another machine or checkout can re-resolve. The `@` mention
is documented as an accelerator rather than the mechanism: official docs state an
`@` path can be relative or absolute but document no drive-letter or
whitespace-bearing form, so the directive stays actionable when expansion does
not fire.

find-handoff inherited the same single-root assumption. Its detection contract
now accepts both the rooted and the legacy rootless form, matching on the shape
they share and diverging only at the existence check, so the corpus already on
disk keeps recovering. A rootless path that resolves to nothing is UNRESOLVED
rather than discarded: one bounded read-only widening over repository roots
already in hand, then surfaced at the confirm gate with its directive verbatim.

Rung 1's repo-correlation check stays open and now says so in place: a handoff
file records no durable repository identity, and adding one is a cross-cutting
frontmatter schema change decided on its own merits.

Refs #1644

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
…ked decision

The gap statement referenced the defect it was split from rather than the item
that carries the decision. #1778 holds the repo-identity frontmatter question
with its options; cite that instead. Also makes find-handoff eval 1's
cwd-resolution expectation form-aware, so it no longer asserts that every
directive path is relative.

Refs #1644

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Read changed files
  • Review reference/save-point.md (producer changes)
  • Review skills/find-handoff/SKILL.md (consumer changes)
  • Review skills/handoff/SKILL.md
  • Review eval files
  • Review plugin.json and CHANGELOG.md
  • Post comprehensive review

Summary

This is a well-scoped, internally consistent fix for a real failure mode. The root cause — a rootless path in the resume prompt resolving against the wrong directory — is accurately diagnosed, the producer fix is correct, and the consumer fix correctly handles backward compatibility without dropping the legacy corpus. The gap is honestly documented. I have a few observations below, none of which block.


Findings

1. Producer save-point.md — logic is sound

The new template shape (line 197–201) is correct:

Read @<handoffs-dir>/<TS>-handoff-<topic>.md and continue its remaining next steps.
Prior session: <UUID>.
Handoff origin: <repo-identity>, relative path <memory_dir>/handoffs/<TS>-handoff-<topic>.md.

The rationale for forward-slash normalization (backslashes are double-escaped in JSONL, which find-handoff greps) is a real constraint and is documented. The @ caveat (drive-letter and whitespace-bearing paths not confirmed by official docs) is appropriately flagged as unverified and the directive is designed to stay actionable without @ expansion. ✓

Minor observation: The spec says Handoff origin: is "computed at emit time from the repository actually written into" (lines 236–241) but doesn't add explicit guidance for the scenario where cwd ≠ worked-in repo — that is, which repo should be identified as the origin when the session was launched from a different directory. The existing language ("the repository actually written into") is the right frame, but the cross-repo scenario (eval #4 in handoff/evals/evals.json exactly covers this) relies entirely on the model inferring from context that the repo it has been writing files into is the target. This is probably sufficient, but a one-line note like "when cwd is not the repo the writes went into, name the repo the file was actually written to" would make the edge case explicit.

2. Consumer find-handoff/SKILL.md — both-form acceptance is correct

The two-form diverge-only-at-existence-check approach (lines 163–183) is exactly right: match on the shared …handoffs/<TS>-handoff-… shape, then:

  • Rooted path → check as given
  • Rootless path → resolve against source transcript cwd, and if not found → UNRESOLVED, never dropped

The bounded widening (current repo root + transcript cwd, never machine-wide) is a proportionate response before surfacing UNRESOLVED. ✓

The OPEN gap statement (lines 129–141) is the right call — stating the gap in place means the next reader can't mistake it for closed. The rejection of the transcript-based substitute (absent transcript = no check, rootless legacy = no check, i.e. fails exactly where it's needed) is sound reasoning.

3. handoff/SKILL.md checklist — correctly tightened

The updated checklist item (lines 143–148) now explicitly prohibits the bare relative segment and requires the absolute forward-slash-normalized path plus the Handoff origin: line. The language is clear and actionable.

4. Eval coverage is appropriate

The eval set covers the happy paths and the boundary conditions that matter most.

5. CHANGELOG and version bump

CHANGELOG entry is accurate, well-scoped, and correctly documents the known gap pointing to #1778. Version bump 0.17.19 → 0.17.20 is appropriate for a fix. ✓


Non-blocking notes

  • Forward-slash normalization is enforcement-free. Since this is prose instruction, the model is asked to normalize paths from \ to / without a shell command doing it. This is inherent to the instruction-based approach and the rationale (JSONL double-escape) is documented, but there's no lint/test that can catch a model that emits backslashes on Windows. This is a known limitation of the medium, not a defect.
  • Handoff origin: comma delimiter — the format <repo-identity>, relative path … uses a comma. Origin remote URLs don't contain commas, and root directory names rarely do, so in practice this is fine. If this line is ever parsed programmatically, the delimiter choice may need revisiting.

Verdict

The fix is correct and the scoping decisions (what's in, what's out, and why #1778 is out) are well-justified. Backward compatibility is handled correctly, the failure mode is diagnosed precisely, and the CHANGELOG honestly documents the remaining gap. Ready to merge as-is; the minor observation about explicit cross-repo Handoff origin: guidance is worth noting but doesn't need to block.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780

  • Gather PR diff and changed files
  • Review reference/save-point.md — path emission logic
  • Review skills/find-handoff/SKILL.md — path resolution and dual-form acceptance
  • Review skills/handoff/SKILL.md — directive construction
  • Review eval files for trust boundary issues
  • Review plugin manifest for version/config surface
  • Synthesize findings

Findings

IMPORTANT · PLAUSIBLE — Handoff origin: line can embed git remote credentials

Location: plugins/session-flow/reference/save-point.md lines 235–240

The new <repo-identity> spec instructs the model to use "the repository's origin remote URL when it has one." Git remote URLs regularly carry embedded credentials in the userinfo component — https://ghp_TOKEN@github.com/org/repo.git for HTTPS-with-PAT and https://user:TOKEN@host/... for any HTTPS remote configured with a stored password. This pattern is common in CI toolchains, credential-helper configurations on Windows, and pipelines that embed tokens for access to private repositories.

The Handoff origin: line is inside the dashed rails — the PR explicitly states "It sits inside the rails because the copy region is what travels." That means any embedded credential lives in the region the user is instructed to copy-paste, and travels:

  1. Into the new session's context
  2. Onward to any other session or machine the prompt is forwarded to

The general redaction pass is the sole defense (save-point.md "Redaction pass": sweep for "tokens, credentials, connection strings, and PII"). A capable model should recognize user:TOKEN@ in a URL as a credential, but:

  • The spec does not instruct the model to strip the userinfo component from the URL before embedding it — the instruction is simply to embed "the repository's origin remote URL"
  • The redaction pass is a model-driven heuristic, not a structured sanitizer; less-obvious token forms (ghp_... without the user: prefix, or OAuth tokens in the x-access-token:TOKEN@ pattern) have no guarantee of detection
  • A failure in the pass embeds live credentials in an artifact designed to persist and travel

Recommended fix: Add an explicit instruction in the <repo-identity> definition to strip the userinfo segment before embedding: "run git remote get-url origin, remove any user:password@ portion (the userinfo component, :// up to and including the @), and embed the sanitized URL." Belt-and-suspenders: the redaction pass then catches any residual, but the credential is not in the string to begin with. A sentence in the redaction-pass section calling out remote URLs explicitly as a known vector would also help.

Suggested fix →


No Issues Found

The following surface areas were reviewed and raised no security concerns:

  • find-handoff path resolution logic — The dual-form acceptance (rooted vs rootless directive) and the UNRESOLVED promotion path are read-only throughout. The bounded widening search is explicitly scoped to "repository roots already in hand — the current repo and the cwd recorded by the candidate transcript — never a machine-wide filesystem sweep." No injection surface from directory names or cwd values; the logic is model-driven, not shell-executed.

  • Transcript JSONL scanning — The scan is bounded (mtime-sorted top-N candidates), excludes the current session's own file, and the per-candidate existence check is a file-system read, not an execution. The false-positive filter (template placeholder tokens) rejects the right set without blanket-rejecting valid angle-bracket content (<REDACTED: …> shape markers). The role-field pre-filter plus per-candidate decode correctly rejects rails appearing in user messages, tool results, and assistant tool_use inputs.

  • find-handoff redaction requirement — The spec explicitly requires the redaction pass to apply to loop re-arm notes when surfaced (since those quote the operator's original prompt verbatim), preventing loop prompts that carried secrets from propagating unchecked.

  • No GitHub Actions changes — No workflow files were modified; supply-chain and dangerous-trigger concerns do not apply to this PR.

  • Plugin manifest — Only a semver bump (0.17.190.17.20); no new config surface, permission grants, or remote MCP endpoints.

@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: bf8d27ffc5

ℹ️ 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/skills/find-handoff/SKILL.md Outdated
Comment thread plugins/session-flow/skills/find-handoff/SKILL.md Outdated
…op calling the origin line a signal

Review found the rooted path had no not-found rule. `Handoff origin:` was only
consulted on the rootless branch — where the contract says it can never appear,
since it shipped with the rooted form — so the case it exists for was the one
case nothing handled: a resume on a different machine or checkout, where the
absolute path is real but local to somewhere else. With no rule, that fell
through to discard-on-miss, reintroducing the defect this change removes, one
path over.

The existence check now branches by path form and converges on one rule. A
rooted miss reads the origin line and re-resolves against the repository it
names; a rootless miss resolves against the source transcript's cwd as before.
Either that lands, or the candidate is UNRESOLVED — and the confirm gate names
which failure it was, since "the path has no root" and "nothing is at that
absolute path on this machine" send an operator to different places, and
"missing" sends them nowhere.

`Handoff origin:` is also no longer described as a fourth signal. It cannot
admit or reject a candidate, so it was never a detection key, and the label
collided with the one the `/loop` re-arm note already holds. It is a resolution
input, read after a candidate qualifies, and the signal summary says so instead
of listing three signals while the ladder depends on a fourth thing.

Adds a rooted-miss eval case, pins the rootless case to its branch explicitly,
renames the changelog's non-standard `Known gaps` heading to the precedented
`Notes`, and shows a POSIX path beside the Windows one.

Refs #1644

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Check what changed since last review (new commits since bf8d27f)
  • Review the new commit 6c256ae changes
  • Review reference/save-point.md changes
  • Review skills/find-handoff/SKILL.md changes
  • Synthesize findings and post review

What changed since the prior review

One new commit (6c256ae) landed after the prior two reviews: "fix(session-flow): give a rooted directive miss a disposition, and stop calling the origin line a signal." This directly addresses Codex reviewer P2 finding #1 — that a rooted path missing on the current machine was being dropped rather than receiving UNRESOLVED treatment.


Assessment of 6c256ae

The core fix is correct

The prior producer shape solved the wrong-root problem for new handoffs but opened a gap: a consumer that checked an absolute path, found nothing, and stopped would silently drop the candidate on a cross-machine or cross-checkout resume — which is the entire class of machines where the new rooted path can fail. This commit closes that gap with four coordinated changes.

Detection contract (reference/save-point.md lines 278–287).

The Handoff origin: line is reclassified from "conditional signal" to "resolution input, not a detection signal." This is the right call: a line that cannot admit or reject a candidate does not belong in the signal list. The consumer contract now states explicitly that a rooted miss is the same not-found-here condition as a rootless one and must get the same UNRESOLVED treatment. The instruction to re-resolve via Handoff origin: before falling through to UNRESOLVED is the correct order: try the cheap local resolution first, surface UNRESOLVED only if that also fails.

Consumer ladder (skills/find-handoff/SKILL.md lines 178–203).

The rooted-directive bullet is now complete: check as given, on miss read Handoff origin: and re-resolve, then fall through to the shared UNRESOLVED rule. The rootless-directive bullet correctly removes the reference to Handoff origin: — legacy blocks pre-date the rooted form and carry no such line. The unified "path that resolves to nothing — rooted or rootless — is UNRESOLVED, never dropped" rule at line 191 is the right abstraction; it prevents either branch from silently discarding.

The widening scope at line 200 is extended to include "the repository named by any Handoff origin: line," which is a bounded addition. The widening is still constrained to "repository roots already in hand" — but see the note below.

UNRESOLVED presentation (lines 284–295).

Separate diagnostic messages for rootless ("path has no root, say which cwd was tried") vs. rooted ("nothing at that absolute path on THIS machine, say the origin repository") is the right UX. The distinction prevents the reporting-absence-for-wrong-reason failure that #1644 identified as the most expensive to diagnose.

Eval coverage.

Eval #10 is correctly updated to remove the Handoff origin: expectation from a pre-rooted legacy block. Eval #11 (rooted miss → re-resolve via Handoff origin:, UNRESOLVED if that also fails) directly exercises the new behavior, including the "A successful re-resolution surfaces the recovered file normally; a failed one surfaces the candidate marked UNRESOLVED" distinction that the prose leaves implicit. The eval makes that conditional explicit, which is the right place to do it.


One minor prose ambiguity

Line 183–185:

on a rooted miss, read that line and re-resolve its repo-relative path against the repository it names, then fall through to the shared rule below

"Fall through" reads as unconditional — a reader could interpret it as "re-resolve AND THEN always UNRESOLVED." The correct reading (and what eval #11 verifies) is "fall through only if that re-resolution also finds nothing." The conditional is implied by context but is not stated. Eval #11 makes it explicit; the prose could too, with "if that also finds nothing, fall through to the shared rule below." Non-blocking — the eval is the right enforcement point for behavior — but noting it for completeness.


Codex P2 finding #2 (not addressed — pre-existing)

The Codex reviewer's second P2 ("Restrict widening to a verified repository root") pointed out that a session started from $HOME sets cwd to a non-git directory, and widening under it could traverse most of the user's filesystem. That concern is pre-existing — the cwd in the widening set appeared before this commit. This commit adds the Handoff origin: repository to the widening set (a bounded addition), but does not change the existing behavior of widening under the transcript cwd without first verifying it's a git root. The security review's credential-leak finding was already noted in the prior reviews and filed separately.


Verdict

The new commit correctly closes the gap the Codex review identified: rooted paths that don't exist locally now get UNRESOLVED treatment via Handoff origin: re-resolution, with the same bounded widening and operator-facing diagnostics already in place for the rootless case. The terminology cleanup (resolution input vs. signal) removes an ambiguity that could have caused a future consumer to treat Handoff origin: as a detection key. Eval #11 directly exercises the critical new path. Ready to merge.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780

  • Read PR diff and changed files
  • Review reference/save-point.md — credential embedding, path construction
  • Review skills/find-handoff/SKILL.md — path resolution logic (all three commits)
  • Review skills/handoff/SKILL.md — directive construction
  • Review eval files — trust boundary issues
  • Synthesize findings and post report

Finding 1 — IMPORTANT · PLAUSIBLE

Handoff origin: embeds raw git remote URLs, and commit 6c256ae makes them more reachable

The prior security review flagged the producer embedding origin remote URLs verbatim into the Handoff origin: line. That finding is still unaddressed. Commit 6c256ae (fix(session-flow): give a rooted directive miss a disposition) amplifies it in two concrete ways.

What 6c256ae adds:

  1. On a rooted miss, find-handoff now explicitly reads the Handoff origin: line value and uses it to re-resolve the repo-relative path. This means the URL (potentially carrying https://ghp_TOKEN@github.com/… or https://user:password@host/… credential forms) is actively consumed as a resolution input.

  2. The UNRESOLVED report at the confirm gate now explicitly surfaces the Handoff origin: value verbatim: "Say the absolute path that was tried, and the repository the Handoff origin: line named." (find-handoff/SKILL.md lines 290–295). A credential-bearing URL shown at this gate is visible output, not transit content.

  3. The bounded widening now adds a third root: "the repository named by any Handoff origin: line" (find-handoff/SKILL.md line 200). If the Handoff origin: contains a URL with embedded credentials, that URL is passed through three surfaces: the transcript (where it's already a concern), the confirm gate display, and the widening path derivation.

Why the redaction pass is insufficient defense here:

find-handoff's redaction spec (lines 325–334) covers "secret / token / credential / connection string / PII." A PAT in https://ghp_TOKEN@… is recognizable, but the x-access-token:TOKEN@ OAuth form and short bearer tokens without the user: prefix are less obvious. More importantly, neither save-point.md's redaction pass nor find-handoff's redaction pass names URL userinfo as an explicit redaction vector — so neither model invocation has a concrete instruction pointing there.

The root is in the producer. save-point.md lines 238–239:

the repository's `origin` remote URL when it has one, else its root directory name

No instruction to strip the userinfo component before embedding.

Recommended fix: In save-point.md, in the <repo-identity> definition, add an explicit sanitization step before embedding the remote URL: run git remote get-url origin, strip any ://user:password@ or ://token@ portion (the userinfo component — everything from :// up to and including the @), and embed the sanitized result. Belt-and-suspenders: also name "remote URLs" explicitly in save-point.md's redaction pass, and add the same note to find-handoff's redaction section since the value is now actively surfaced at the confirm gate.

Fix this →


No Further Issues Found

The following were reviewed and raised no additional security concerns:

  • find-handoff path resolution — read-only throughout. The dual-form existence check added in commit 6c256ae is a model-driven read (not shell-executed), so no command injection surface. The widening is bounded to named roots and never machine-wide.
  • Transcript scanning scope. The cross-project scan is intentionally broad but correctly scoped: bounded to ~/.claude/projects/*/, mtime-sorted and capped, with the current session excluded. Only resume prompt + metadata (redacted) is surfaced, not raw transcript content.
  • Template-placeholder false-positive filtering. The filter rejects the known placeholder tokens but does not blanket-reject angle brackets, correctly preserving <REDACTED: …> markers and generic syntax like <T>.
  • find-handoff redaction. The re-arm note is correctly called out as requiring the same redaction pass since it quotes the operator's original loop prompt verbatim.
  • No GitHub Actions changes. No workflow files were modified; supply-chain and dangerous-trigger concerns do not apply.
  • Plugin manifest. Semver bump only; no new config surface, permission grants, or remote MCP endpoints.

kyle-sexton and others added 2 commits July 30, 2026 08:55
… before globbing under it

`Handoff origin:` embedded the `origin` remote URL verbatim, and an HTTPS
remote routinely carries its credential in the URL's userinfo component — a
PAT, a stored password, or a credential helper's `x-access-token:<token>@`.
The line sits inside the rails, in the region the operator is told to copy,
so an embedded secret travels into the next session and onto every machine
the prompt reaches. The producer now strips everything from `://` up to and
including the `@` before embedding what is left, and falls back to the
repository's root directory name when a URL cannot be sanitized with
confidence. Both redaction passes name remote URLs as an explicit vector,
because a token in a URL reads as one more path segment — the shape a
model-driven sweep is likeliest to walk past — and `find-handoff` applies
the same check to the value it surfaces at the confirm gate and derives a
widening root from, since a recovered handoff predates this rule as easily
as it predates the rooted path.

The bounded widening globbed a filename under a transcript `cwd` it never
verified was a repository root. A session launched straight from a home
directory records that home directory as its `cwd`, so the glob recursively
walks most of the user's files — the machine-wide scan the rule forbids,
reached by accident rather than by intent, and slow enough to time the
recovery out. A `cwd` now earns a place in the widening set only once
`git -C <cwd> rev-parse --show-toplevel` confirms it, and the search runs
under the top level that prints rather than under `cwd` itself. A `cwd` with
no top level contributes no root; the candidate stays UNRESOLVED and the
operator is asked which checkout to look in.

Also: the rooted-miss ladder said "then fall through to the shared rule
below", which read as unconditional — the fall-through is now stated as
conditional on the origin re-resolution also finding nothing, matching what
eval 11 already verified. `<repo-identity>` states the cross-repo case
explicitly (name the repository written into, never the one cwd sits in).
The handoff eval's scenario path is a portable placeholder rather than a
concrete checkout root, which the machine-specific-paths hygiene gate
rejects. Evals 12 and 5 cover the widening-root verification and the
credential stripping; eval 10's widening expectation is corrected, since its
own scenario is the home-directory cwd the rule now refuses to glob under.

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Check what changed since last review (new commits since 6c256ae)
  • Review commit a9ee2dc — sanitize origin URL and verify widening root
  • Synthesize findings and post review

What changed since the prior review

One new commit (a9ee2dc) landed: "fix(session-flow): sanitize the origin URL and verify a widening root before globbing under it."

This commit directly closes the two outstanding security/correctness findings from prior reviews:

  1. Security finding (IMPORTANT · PLAUSIBLE)Handoff origin: embedded git remote URLs verbatim; a remote URL routinely carries a credential in its userinfo component.
  2. Codex P2 finding fix: correct plugin-system inaccuracies and harden references #2 — The bounded widening globbed a filename under a transcript cwd it never verified was a repository root, risking a recursive home-directory sweep.

It also addresses the minor prose ambiguity noted in the second review (conditional fall-through) and the minor observation about making the cross-repo cwd ≠ worked-in-repo case explicit in the <repo-identity> spec.


Assessment of a9ee2dc

1. Credential stripping — producer (save-point.md lines 252–262)

The producer now has an explicit sanitization instruction: run git remote get-url origin, strip everything from :// up to and including the @, and embed the result. A PAT-bearing remote is emitted as https://github.com//.git. When the URL can't be sanitized with confidence, fall back to the root directory name. The reasoning is correctly ordered: strip at emit time so the redaction pass has nothing left to catch; the redaction pass is the backstop, not the mechanism. The three known credential forms (ghp_TOKEN@, user:TOKEN@, x-access-token:TOKEN@) are all caught by the //@` strip.

Correct.

2. Credential stripping — consumer (find-handoff/SKILL.md lines 344–350)

The consumer applies the same userinfo-strip to the Handoff origin: value before surfacing it at the confirm gate and before deriving a widening root from it. The rationale is explicit: a recovered handoff predates the producer's new sanitization rule as easily as it predates the rooted path, so recovery is exactly where an unsanitized URL arrives. Belt-and-suspenders is the right model when the failure is credential exposure.

Correct.

3. Widening root verification (find-handoff/SKILL.md lines 200–212)

A transcript's cwd now earns its place in the widening set only after git -C <cwd> rev-parse --show-toplevel confirms it. Critically, the glob then runs under the top level printed, not under cwd itself — so a session launched from a subdirectory of a repo still works correctly (the handoffs live at the repo root, not in the subdirectory). A cwd with no git top level contributes nothing; the candidate stays UNRESOLVED and the operator is asked which checkout to look in. This is the honest answer when nothing in hand can name one.

The "current repo" root stays unconditionally in the widening set — it is already verified by definition. Only the inferred-from-transcript cwd is gated. ✓

Correct.

4. Conditional fall-through (find-handoff/SKILL.md lines 183–187)

The prior "then fall through to the shared rule below" is now "a hit there surfaces the recovered file normally, and only if that re-resolution ALSO finds nothing does the candidate fall through to the shared rule below." The condition is now explicit in prose, matching what eval 11 already verified behaviorally.

Correct.

5. Eval coverage

  • Eval 5 (handoff) — new; covers credential stripping at emit time, the inside-the-rails reasoning, and the fallback to root directory name. Expectations are well-scoped.
  • Eval 12 (find-handoff) — new; covers the home-directory cwd case end-to-end: cwd has no git top level, contributes no widening root, never recursively globbed, candidate stays UNRESOLVED. The expectations also verify that the current repo and any Handoff origin: repository remain in the set (narrowing cwd never removes verified roots).
  • Eval 10 update — correctly removes the expectation that a home-directory cwd would be searched, since it now won't be.
  • Eval 4 prompt — the concrete checkout path (D:/repos/acme/app) replaced with D:/repos/<owner>/<repo>, fixing the machine-specific-paths hygiene gate failure.

All four changes are correct and well-targeted.

6. CHANGELOG

Four bullet points under [0.17.20] — one for the original rootless-path fix, one for the consumer both-forms fix, one for the widening-root verification, and one for the credential stripping. Each is accurately described. The gap note for #1778 is preserved. ✓


No Issues Found

All findings from prior reviews are closed:

Prior finding Status
Security: Handoff origin: embeds raw credential-bearing remote URLs Fixed — userinfo stripped at producer; same strip applied in consumer before surfacing/widening
Codex P2: widening globs under unverified cwd (home-directory sweep risk) Fixedgit rev-parse --show-toplevel gate before any glob; home-dir cwd contributes no root
Prose: conditional fall-through stated as unconditional Fixed — "only if that re-resolution ALSO finds nothing"
Minor: cross-repo cwd ≠ repo case not explicit in <repo-identity> Fixed — "when cwd is NOT that repository, name the repository the file was actually written to"

Verdict

All previously identified findings are resolved. The implementation is correct and internally consistent end-to-end: the producer sanitizes before emitting, the consumer re-sanitizes on recovery for legacy handoffs, the widening is now genuinely bounded to verified repository roots, and the evaluation suite exercises every new branch. Ready to merge.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780 (latest commit a9ee2dc)

  • Gather PR diff and review latest commit changes
  • Review reference/save-point.md — URL sanitization, credential embedding
  • Review skills/find-handoff/SKILL.md — path resolution, widening scope, credential surfacing
  • Review eval files — trust boundary issues
  • Synthesize and post findings

Prior findings — resolved

Both issues raised by prior security reviews are addressed in commit a9ee2dc.

Credential embedding in Handoff origin:. The producer (save-point.md lines 252–262) now explicitly instructs the model to strip the userinfo component before embedding the URL (://…@ removed), fall back to the root directory name when sanitization is uncertain, and name remote URLs explicitly in the redaction pass as the shape most likely to evade a generic sweep. The consumer (find-handoff/SKILL.md lines 344–350) applies the same check before surfacing the value at the confirm gate or using it as a widening root, covering legacy handoffs that predate the producer fix. Belt-and-suspenders: strip at emit time, strip again before display, redaction pass as the final backstop. ✓

Unbounded widening under unverified cwd. The widening rule (find-handoff/SKILL.md lines 202–207) now requires git -C <cwd> rev-parse --show-toplevel before admitting a transcript cwd to the search set, and globs under the top level that command prints rather than under cwd itself. A cwd with no git top level (home directory, temp dir) contributes no root and is never recursively globbed. ✓


No new security issues found

The following surfaces were reviewed in the full PR diff; none raised concerns:

  • Trust boundaries on transcript content. JSONL scanning is model-driven, not shell-executed. A crafted cwd value in a transcript cannot directly inject a shell command; the worst a hostile transcript can do is supply a cwd that fails the git rev-parse check, which is handled by the UNRESOLVED path rather than a silent discard.

  • Handoff origin: as resolution input. The value is sanitized (userinfo stripped) before being surfaced or used for widening. The relative path component of the line carries no credentials and is the operative re-resolution input.

  • UNRESOLVED surfacing at the confirm gate. The UNRESOLVED report (find-handoff/SKILL.md lines 289–303) surfaces the directive verbatim and the Handoff origin: value — both go through the credential sweep before display. The redaction pass also covers re-arm notes that quote the original loop prompt verbatim.

  • Plugin manifest. Semver bump only (0.17.190.17.20); no new config surface, permission grants, or remote MCP endpoints.

  • No GitHub Actions changes. No workflow files modified; supply-chain and dangerous-trigger concerns do not apply.

  • Eval files. The new evals (handoff feat(hook-telemetry): marketplace-wide telemetry contract + markdown-formatter producer #5, find-handoff fix(comment-hygiene): exclude '.' from the owner/repo#N leading boundary #12) exercise precisely the credential-stripping and widening-root-verification behavior. No trust boundary issues in the eval data itself.

Ready to merge from a security standpoint.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification of PR Review comment 5126562602 (findings extracted individually; sections 1–5's ✓ affirmations carry no work item).

# Finding Classification Evidence Reacted
1 <repo-identity> says "computed at emit time from the repository actually written into" but adds no explicit guidance for cwd ≠ worked-in repo — the cross-repo case (handoff eval 4) relies on the model inferring it VALID — fixed in a9ee2dc4 save-point.md <repo-identity> now states it inline: "computed at emit time from the repository actually written into — when cwd is NOT that repository, name the repository the file was actually written to, never the one cwd happens to sit in". Cheap and the PR introduced the line, so it is fixed rather than noted. 👍
2 Forward-slash normalization is enforcement-free — no lint or test can catch a model emitting backslashes on Windows INCORRECT (not a defect) Agreed on the fact and it is not a defect in this change, as the finding itself concludes ("a known limitation of the medium, not a defect"). The rationale is documented at the instruction site, and the consumer does not depend on the producer having normalized: find-handoff matches the directive on its …handoffs/<TS>-handoff-… shape, so a backslash-bearing path still qualifies as a candidate and still reaches the existence check. No code path treats normalization as a precondition, so there is nothing to harden. 👍
3 Handoff origin: uses a comma delimiter (<repo-identity>, relative path …); if ever parsed programmatically the choice may need revisiting INCORRECT (not a defect) Nothing parses this line, by contract rather than by omission: save-point.md classifies it as a "resolution input, not a detection signal — it cannot admit or reject a candidate", and find-handoff reads it only after a candidate has already qualified. Committing to a machine-parseable delimiter now would be designing for a consumer that does not exist and would harden a shape the detection contract deliberately does not key off. Recorded here rather than tracked: the trigger is a consumer that actually parses the line, and that consumer's own change is where the delimiter gets decided. 👍

Pushed as a9ee2dc4.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification of Security Review comment 5126563136.

# Finding Classification Evidence Reacted
1 IMPORTANT · PLAUSIBLE — <repo-identity> instructs embedding the origin remote URL with no sanitization step, so an HTTPS remote's userinfo credential (https://<token>@host/…, https://<user>:<token>@host/…, x-access-token:<token>@) lands inside the rails, i.e. inside the copy region that travels to other sessions and machines; the model-driven redaction pass is the sole defense and never names URL userinfo as a vector VALID — fixed in a9ee2dc4 Verified: the pre-fix text read "the repository's origin remote URL when it has one, else its root directory name" with no sanitization, and the Handoff origin: line is emitted inside the rails. Not deferrable under any reading — the line is introduced by this PR, so this is a credential-exposure path the change itself ships. Fix follows the recommendation: save-point.md <repo-identity> now requires stripping everything from :// up to and including the @ before embedding, with a fallback to the repository's root directory name when a URL cannot be sanitized with confidence (identity preserved, secret absent). Belt-and-suspenders as suggested: save-point.md's "Redaction pass" now names git remote URLs as an explicit vector, with the reason a generic sweep misses them — a token in a URL reads as one more path segment. handoff eval 5 (handoff-origin-strips-remote-url-credentials) asserts the behavior. 👍

Pushed as a9ee2dc4.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification of PR Review comment 5126993370 (the 6c256ae re-review).

# Finding Classification Evidence Reacted
1 Prose ambiguity — "then fall through to the shared rule below" reads as unconditional, i.e. "re-resolve AND THEN always UNRESOLVED"; the correct reading is "fall through only if the re-resolution also finds nothing", which eval 11 asserts but the prose does not state VALID — fixed in a9ee2dc4 The rooted-directive bullet now reads: "re-resolve its repo-relative path against the repository it names; a hit there surfaces the recovered file normally, and only if that re-resolution ALSO finds nothing does the candidate fall through to the shared rule below." Agreed that the eval is the enforcement point, but the prose is the instruction a model actually executes, so the conditional belongs in both. 👍
2 Codex P2 #2 ("Restrict widening to a verified repository root") is characterized as pre-existing — "the cwd in the widening set appeared before this commit" — with 6c256ae only making a bounded addition INCORRECT The claim is right about 6c256ae and wrong about the PR. git show origin/main:plugins/session-flow/skills/find-handoff/SKILL.md | grep -iE 'widen|glob that filename|UNRESOLVED|repository roots already in hand' returns no widening instruction on the base branch — the only widen hit is unrelated prose about bounding rail search. The bounded widening, the UNRESOLVED disposition, and the transcript-cwd resolution are all introduced by this PR; cwd predates 6c256ae but not main. That distinction decides the disposition: a defect that does not reproduce on the base branch is one this change introduces, so it is VALID (fix now) and not eligible for deferral regardless of its P2 badge. Fixed in a9ee2dc4 — see the threaded reply on that finding. 👍

Pushed as a9ee2dc4.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification of Security Review comment 5126996334 (the 6c256ae re-review).

# Finding Classification Evidence Reacted
1 IMPORTANT · PLAUSIBLE — the prior credential finding is still unaddressed, and 6c256ae amplifies it three ways: the Handoff origin: value is now actively consumed as a resolution input, surfaced verbatim at the UNRESOLVED confirm gate, and used to derive a third widening root; neither redaction pass names URL userinfo as a vector VALID — fixed in a9ee2dc4 All three amplification paths confirmed on the branch. Producer root fixed at the source: save-point.md <repo-identity> now requires stripping everything from :// up to and including the @ before embedding, falling back to the root directory name when a URL cannot be sanitized with confidence. Both belt-and-suspenders items applied as recommended — save-point.md's "Redaction pass" names git remote URLs as an explicit vector, and find-handoff's "Read-only + redaction — hard invariants" gains a bullet making Handoff origin: a named credential vector, checked for an @ ahead of its host and reduced to bare scheme-and-host before it is shown at the confirm gate or used to derive a widening root. The consumer-side check is not redundant with the producer fix: a recovered handoff predates this rule as easily as it predates the rooted path, and recovery is precisely where an unsanitized one arrives. handoff eval 5 asserts the producer behavior. 👍

Pushed as a9ee2dc4.

…hecklist

The credential-stripping rule lives in the engine spec's `<repo-identity>`,
but the checklist item that emits the `Handoff origin:` line is the surface
the producer actually ticks. Naming the requirement there — as a qualifier,
not a restatement of the rationale — closes the gap where a model satisfies
the checklist without reading the engine doc's definition.

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Check what changed in the new commit (ae3a497) since last review
  • Review affected files at HEAD
  • Post findings

What changed since the prior review

One new commit (ae3a497) landed: "fix(session-flow): name the origin-URL sanitization on the producer checklist." One file changed — skills/handoff/SKILL.md, one line.


Assessment of ae3a497

The change

The enforcement checklist item in handoff/SKILL.md line 144–146 now reads:

- with the `Handoff origin:` line naming the repository and
- repo-relative path a different machine re-resolves from
+ with the `Handoff origin:` line naming the repository
+ (a remote URL with its userinfo credential stripped) and repo-relative path a different machine
+ re-resolves from

Why this closes a real gap. Commit a9ee2dc landed the full credential-sanitization spec in save-point.md's <repo-identity> definition and redaction-pass section. But the enforcement checklist in handoff/SKILL.md — the item a model executing the handoff skill actually ticks off before stopping — still only said "naming the repository and repo-relative path," with no mention of sanitization. A model that checks this item without having read the full <repo-identity> spec might emit an unsanitized URL and still mark the box. This commit closes that gap: the checklist item now names the security property as a required property of the line, making it verifiable at the point of emission.

The change is correct. The parenthetical (a remote URL with its userinfo credential stripped) qualifies the remote-URL form of <repo-identity>. The fallback (root directory name, emitted when a URL cannot be sanitized with confidence) has no userinfo component to strip, so the parenthetical doesn't mislead for that case — it simply doesn't apply.

One minor prose note (non-blocking). The sentence now reads "naming the repository (a remote URL with its userinfo credential stripped)" — which a careful reader could parse as "the repository is always a remote URL." Per the spec in save-point.md, it may instead be the root directory name. In practice the common case is the URL form and the parenthetical correctly captures what is required there; the edge case is covered by save-point.md. A reader who follows up will find the fallback. This is a documentation nuance, not a correctness issue.


Verdict

All findings from prior reviews remain closed. The new commit is a targeted, correct addition that makes the credential-stripping requirement visible at the checklist level, so a model executing the handoff skill can verify it at emission time rather than only by reading back through the full <repo-identity> spec. Ready to merge.


@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780 (latest commit ae3a497)

  • Gather PR diff and latest commit changes
  • Review reference/save-point.md — redaction pass, credential stripping, <repo-identity>
  • Review skills/find-handoff/SKILL.md — path resolution, widening scope, Handoff origin: surfacing
  • Review skills/handoff/SKILL.md — latest commit checklist change
  • Synthesize findings

What ae3a497 changes

One file: plugins/session-flow/skills/handoff/SKILL.md. The checklist item for emitting the Handoff origin: line previously named "the repository and repo-relative path" — enough for a model working from the checklist alone to embed an unsanitized remote URL without ever consulting <repo-identity> in the engine spec. The patch adds "(a remote URL with its userinfo credential stripped)" as a qualifier on the repository reference, closing that gap.

This is the right fix for the right surface. The checklist is where the producer model ticks off work; the engine doc is where the rationale lives. A model that follows the checklist without reading the engine doc would now still strip credentials.


Prior findings — all resolved

All findings from the three prior security review passes are closed and verified in this pass:

Finding Resolution
Handoff origin: embeds raw git remote URLs (producer) Fixed a9ee2dc<repo-identity> now requires stripping ://…@ before embedding; redaction pass explicitly names remote URLs as a vector
find-handoff surfaces Handoff origin: value verbatim at confirm gate Fixed a9ee2dc — consumer checks for @ ahead of host and reduces to bare scheme-and-host before surfacing or widening
Widening globs under unverified cwd (home-directory sweep) Fixed a9ee2dcgit -C <cwd> rev-parse --show-toplevel gate before any glob; top-level printed, not cwd itself; home-dir cwd contributes no root
Checklist satisfied without reading credential-stripping rule Fixed ae3a497 — qualifier now in the checklist item itself

No new security issues found

The following were reviewed against the current branch and raised no concerns:

  • save-point.md redaction pass — The pass now names git remote URLs explicitly as the shape most likely to evade a generic sweep. The <repo-identity> definition requires userinfo stripping at emit time, making the redaction pass the backstop rather than the mechanism. Belt-and-suspenders is the correct order. ✓

  • find-handoff path resolution — read-only throughout. The existence check, widening, and git rev-parse gate are all model-driven reads; no shell command is constructed from transcript-sourced content in a way that introduces injection risk beyond what the model's tool call layer already handles. The candidate cwd goes to a git -C flag, not a string-interpolated shell expansion. ✓

  • Handoff origin: as resolution input. The value is sanitized (userinfo stripped) before being surfaced or used to derive a widening root. Legacy handoffs predating the producer fix are handled on the consumer side for exactly this reason. ✓

  • Transcript JSONL scanning. Bounded to ~/.claude/projects/*/, mtime-sorted, capped. Per-candidate decode rather than bulk parse. Role-field pre-filter plus full decode prevents rails in user messages or tool results from qualifying as handoff emissions. ✓

  • Template-placeholder filter. Correctly rejects template tokens without blanket-rejecting angle brackets — preserves <REDACTED: …> shape markers and valid syntax like <T>. ✓

  • Eval files. New evals (handoff feat(hook-telemetry): marketplace-wide telemetry contract + markdown-formatter producer #5, find-handoff fix(comment-hygiene): exclude '.' from the owner/repo#N leading boundary #12) exercise credential stripping and widening-root verification. No trust-boundary issues in the eval data. ✓

  • Plugin manifest. Semver bump only (0.17.190.17.20). No new config surface, permission grants, or remote MCP endpoints. ✓

  • No GitHub Actions changes. No workflow files modified; supply-chain and dangerous-trigger concerns do not apply. ✓


Verdict

The full PR is clean from a security standpoint. The latest commit closes the one remaining gap — the checklist-vs-engine-spec split on credential stripping — that a prior pass identified as implicit rather than stated. All four previously identified security and correctness findings are resolved. Ready to merge.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Follow-up to the two Security Review credential findings (5126563136, 5126996334) — supplementary commit ae3a497f.

a9ee2dc4 fixed the root in the engine spec (save-point.md <repo-identity>) and named remote URLs in both redaction passes. ae3a497f closes the remaining enforcement gap: handoff/SKILL.md's checklist item is the surface the producer actually ticks when it emits the Handoff origin: line, and it named the line without naming the sanitization. It now carries the requirement as a qualifier — "(a remote URL with its userinfo credential stripped)" — rather than a restatement of the rationale, so the rule is visible at the point of emission without duplicating the engine doc.

Verified on the branch: markdownlint-cli2 0 errors; scripts/check-changed-skills.sh origin/mainCHECK-SKILL handoff: PASS — 0 errors, 0 warning(s), 2 skills checked / 0 failed; the machine-specific-paths CI script run locally → "No machine-specific absolute paths detected."

…and bound the stripping

Two adjacent rules prescribed different outputs for the same secret class
with no stated precedence. The general redaction pass says replace a
credential with a shape marker and never the value; the git-URL rule two
sentences later says strip the userinfo and re-embed the bare
scheme-and-host form. A model executing the pass could not tell which
governs the `Handoff origin:` value.

Both passes now state the precedence and the reason. The URL is reduced to
its bare scheme-and-host form and NOT replaced with a marker — a deliberate
exception, because the general rule assumes the whole value is secret and
unneeded downstream, whereas a remote URL's host and path are non-secret and
are exactly what a rooted miss re-resolves from. `<REDACTED: remote URL>`
would trade a credential leak for a broken recovery.

The stripping rule also gets its boundaries. A bare ssh account name
(`ssh://git@host/…`) is not a credential — the secret is the local key the
URL does not carry — so it stays; the literal "strip from `://` to `@`"
would have dropped it for no gain while misstating the remote. And "cannot
be sanitized with confidence" now has a test rather than a vibe: fall back
to the root directory name when the userinfo boundary is undeterminable —
more than one `@` ahead of the path, or the SCP-style
`git@host:<owner>/<repo>.git` form with no `://` to anchor stripping on.
`<repo-identity>`'s summary sentence named only two outcomes and now names
the sanitization condition as well.

Eval 5's expectations are tightened to the credential-bearing userinfo and
assert the bare-host form over a shape marker; eval 6 covers the ssh and
SCP-form branches.

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Check what changed in the new commit (001808c) since last review
  • Review affected files at HEAD
  • Post findings

What changed since the prior review

One new commit (001808c) landed: "fix(session-flow): state which redaction rule wins for a remote URL, and bound the stripping."

Four files changed: reference/save-point.md, skills/find-handoff/SKILL.md, skills/handoff/evals/evals.json, and CHANGELOG.md.


Assessment of 001808c

The problem being fixed

After a9ee2dc added URL sanitization, two adjacent rules in save-point.md prescribed different outputs for the same class: the general redaction pass says replace a credential with a shape marker, while the git-URL-specific rule says strip the userinfo and keep the rest. A model executing both rules in order had no stated precedence. On a Handoff origin: value carrying a credential-bearing remote URL, "which rule governs this?" was genuinely ambiguous.

Precedence rule — correct

save-point.md lines 60–80 and find-handoff/SKILL.md lines 341–360 both now state: drop the userinfo, keep the rest — do NOT replace with a shape marker. This rule wins for this class. The rationale is explicit and correct: the general rule redacts to a marker because the whole value is secret and nothing downstream needs it; here the opposite holds — the host and path are non-secret and are load-bearing for rooted-miss re-resolution. A <REDACTED: remote URL> marker would destroy the identity the line is emitted to carry, trading a credential leak for a broken recovery. This is the right resolution and correctly mirrors across both producer and consumer. ✓

SSH account name exception — correct

save-point.md lines 275–280: ssh://git@host/… keeps its git@. The reasoning is sound — the secret is the local key, which the URL does not contain, so the git@ is a well-known account name and not a credential. The prior ://@ stripping would have emitted ssh://host/…, which misstates the remote without improving security. Staying is the right call. ✓

"Cannot sanitize with confidence" boundary — concrete and correct

save-point.md lines 281–288 replaces "cannot be sanitized with confidence" (a vibe) with a test: fall back to the root directory name when:

  1. More than one @ appears ahead of the path — the userinfo boundary is ambiguous (RFC 3986 allows at most one @ in userinfo, so multiple @s signals a malformed or exceptional form)
  2. No :// to anchor on, as in the SCP-style git@host:<owner>/<repo>.git form

Both conditions are correct. The multi-@ case is a real ambiguity that could arise from proxy chains or credential helpers writing unusual URL forms. The SCP case has no scheme separator, so there is no anchoring point for stripping. In both cases the directory-name fallback is safe and re-resolves nearly as well. ✓

Consumer mirrors the producer — by reference, not duplication

find-handoff/SKILL.md states the same treatment, then defers to `save-point.md` `<repo-identity>` for the precise test condition rather than duplicating it. This is the right approach — the definition of "userinfo boundary undeterminable" is owned in one place. ✓

Eval coverage — appropriate

  • Eval 5 (updated): Now explicitly asserts the bare-host form (not a marker) and names the precedence rationale. The "A URL whose userinfo boundary cannot be determined falls back to the root directory name rather than being emitted as-is or guessed at" expectation is correctly scoped — it covers the fallback without overspecifying the test that triggers it. ✓
  • Eval 6 (new): handoff/evals/evals.json eval 6 covers ssh://git@… (keeps userinfo), SCP-style git@… (no ://, falls back to directory name), and explicitly asserts the distinguishing criterion is credential-bearing vs. non-credential userinfo (not ssh vs. https). The fifth expectation — "neither case is emitted as a <REDACTED: …> shape marker" — directly exercises the new precedence rule. ✓

CHANGELOG

The new entry under [0.17.20] accurately describes the rule conflict, the resolution (precedence + reason), and the boundary conditions. Substance matches the change. ✓


No Issues Found

All prior findings remain closed. This commit is a clean follow-on that resolves the rule ambiguity the prior security fix introduced. No new findings.

Prior finding Status
Rule conflict: general redaction (shape marker) vs. git-URL rule (strip-and-keep) — ambiguous precedence for Handoff origin: Fixed — git-URL rule is explicitly named the winner with stated rationale; mirrors in both producer and consumer
Credential-bearing remote URLs embedded in Handoff origin: Closed in prior commit; unchanged
Widening globs under unverified cwd Closed in prior commit; unchanged
Conditional fall-through stated as unconditional Closed in prior commit; unchanged

Verdict

The commit correctly resolves the precedence ambiguity without disturbing any of the prior fixes. The SSH account name exception and the SCP-style fallback are correctly reasoned, the test for "undeterminable boundary" is concrete, and the eval set covers both new branches. Ready to merge.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780 (latest commit 001808c)

  • Gather context and understand the request
  • Get PR diff to see what the latest commit changed
  • Read changed files
  • Review reference/save-point.md — redaction rules, credential stripping
  • Review skills/find-handoff/SKILL.md — path resolution, credential surfacing
  • Review skills/handoff/SKILL.md — emission contract
  • Synthesize findings and post report

What 001808c adds

This commit resolves a rule-precedence ambiguity and tightens the userinfo-stripping logic:

  1. Precedence stated explicitly — the git-URL strip rule (userinfo dropped, host/path kept) now states it wins over the general shape-marker rule for the specific class of git remote URLs. Prior text gave two prescriptions for the same secret class with no stated winner.
  2. SSH bare account name carved outssh://git@host/… keeps its git@ because an SSH account name is not a credential; the secret is the local private key, which the URL does not contain.
  3. SCP-style and ambiguous URLs fall backgit@host:<owner>/<repo>.git (no :// to anchor stripping) and any URL with more than one @ ahead of the path fall back to the repository's root directory name rather than guessing where userinfo ends.
  4. Eval 6 added — covers the SSH and SCP-form branches.

Prior findings — status

All four previously identified security findings are closed:

Finding Resolution
Handoff origin: embedded raw credential-bearing git remote URLs Fixed a9ee2dc
Widening globbed under unverified cwd (home-directory sweep) Fixed a9ee2dc
Checklist satisfied without reading credential-stripping rule Fixed ae3a497
Two rules prescribed different outputs for the same secret class, no precedence stated Fixed 001808c

One new SUGGESTION — 001808c

Severity: SUGGESTION — Confidence: LOW

"Any URL in the outbound set" scope is slightly broader than the intended git-remote-only exception.

save-point.md lines 66–76:

So any URL in the outbound set is checked for an @ ahead of its host. Drop the userinfo and keep the rest — do NOT replace the URL with a shape marker. This is a deliberate exception to the rule above, and for this one class it wins.

The phrase "any URL in the outbound set" sits in a paragraph scoped to git remote URLs and "this one class" correctly limits the exception. However, a model parsing this literally could apply the strip-and-preserve treatment to any credential-bearing URL it finds anywhere in the handoff body — not just a git remote URL in the Handoff origin: line. For example, a database connection string like mongodb+srv://admin:secret@prod-db.internal/mydb would, under this reading, emit as mongodb+srv://prod-db.internal/mydb rather than <REDACTED: database connection string>. The userinfo credential is removed in both interpretations, so there is no credential leak risk; but the host and path remain visible where the general rule would have redacted them, which is a minor infrastructure-disclosure difference.

The intent is clear from context — the paragraph starts with "Git remote URLs are a named vector", and <repo-identity> separately constrains what appears in Handoff origin: to git remote URLs. But "any git remote URL in the outbound set" would make the scope unambiguous and prevent the parse that applies the strip-and-keep treatment to non-git URLs.

Confidence is LOW because a well-trained model reads contextually (the whole paragraph is about git remote URLs) and because even the worst-case reading still removes the credential. The risk is an information-leak of infrastructure host names, not a credential leak.


No new issues found on other surfaces

  • SSH carve-out is correct. ssh://git@host/… retains git@ safely — no credential is in a Git SSH URL; the authentication secret is the local private key, which the URL does not contain. Dropping the account name would misstate the remote without improving security.
  • SCP-style and multi-@ fallback is safe. Falling back to the root directory name rather than guessing where userinfo ends is the conservative path — it loses URL-based recovery identity, not the secret.
  • Consumer-side rule (find-handoff/SKILL.md lines 344–358) is correctly scoped to "the Handoff origin: value" rather than all URLs, so the scope ambiguity noted above does not appear on the consumer side.
  • No GitHub Actions changes. No workflow files modified; supply-chain and dangerous-trigger concerns do not apply.
  • Plugin manifest. Semver bump only; no new config surface, permission grants, or remote MCP endpoints.
  • Eval 6 covers both branches (SSH stays, SCP falls back) correctly and without trust-boundary issues in the eval data itself.

Verdict

The PR is clean from a security standpoint. The latest commit correctly resolves the rule-precedence ambiguity and tightens the stripping boundaries without introducing new exposure. The one SUGGESTION is a prose-clarity note; the credential is removed in all interpretations, so it does not represent an exploitable gap.

"Any URL in the outbound set is checked for an `@`" was broader than the
git-remote-only exception it introduced. Read literally, a connection string
like `mongodb+srv://<user>:<secret>@<host>/<db>` would take the
host-preserving strip instead of a shape marker — the credential goes either
way, but the host and path would survive where the general rule redacts
them, disclosing infrastructure for nothing.

The rule now says git remote URLs, and states the boundary rather than
leaving it to context: strip-and-keep applies where the remainder is
load-bearing, because recovery re-resolves from a remote's surviving host and
path; nothing re-resolves from a database host, so everywhere else the marker
still wins.

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

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


PR Review — fix(session-flow): root the resume-prompt path and accept both forms on recovery

  • Gather context and understand the request
  • Check what changed in the new commit (24c74ea) since last review
  • Review affected files at HEAD
  • Post findings

What changed since the prior review

One new commit (24c74ea) landed: "fix(session-flow): scope the strip-and-keep exception to git remote URLs."

Two files changed: reference/save-point.md and CHANGELOG.md.


Assessment of 24c74ea

The problem being fixed

The prior security review's final SUGGESTION (confidence: LOW) identified that the redaction-pass rule introduced in 001808c was written with slightly broader scope than intended. The prior text read:

So any URL in the outbound set is checked for an @ ahead of its host. Drop the userinfo and keep the rest — do NOT replace the URL with a shape marker. This is a deliberate exception to the rule above, and for this one class it wins.

Read literally, a connection string like mongodb+srv://admin:secret@prod-db.internal/mydb would take the host-preserving strip instead of a shape marker. The credential would be removed either way, but the host and path would survive where the general rule would redact them — disclosing infrastructure for no recovery benefit. The risk was information-disclosure, not credential-leak.

The fix is correct

save-point.md lines 66–84 makes three coordinated changes:

  1. Scope narrowed at the rule: "any URL in the outbound set" → "every git remote URL in the outbound set"
  2. Exception bound stated explicitly: "for this one class it wins" → "it wins for git remote URLs and nothing else"
  3. Non-generalization paragraph added: A second bold paragraph states the exception explicitly and explains the discriminator — what earns a git remote URL its exception is that something downstream re-resolves from the surviving host and path; nothing re-resolves from a database host. The example (mongodb+srv://…) closes any interpretive gap.

The reasoning is correct and self-consistent. The exception's boundary is now derivable from first principles (load-bearing remainder → strip-and-keep; not load-bearing → marker), not just from context. A model following this rule cannot accidentally apply strip-and-keep to a database connection string even if it reads only the rule paragraph and not the surrounding prose.

Consumer side — no changes needed

find-handoff/SKILL.md was already correctly scoped in 001808c. Its rule names "the Handoff origin: value is a named credential vector" — not "any URL" — so the broader-scope ambiguity existed only on the producer side. That's confirmed by inspecting the current text at lines 344–356: unchanged, correctly targeted.

CHANGELOG

The [0.17.20] entry is updated with one sentence accurately describing the scoping: the exception is stated not to generalize, with the load-bearing rationale preserved. Substance matches the change. ✓


No Issues Found

All findings from prior reviews remain closed:

Finding Status
Handoff origin: embeds raw credential-bearing remote URLs Closed a9ee2dc
Widening globs under unverified cwd Closed a9ee2dc
Checklist satisfied without reading credential-stripping rule Closed ae3a497
Rule conflict: no precedence stated between general (shape marker) and git-URL (strip-and-keep) rules Closed 001808c
Scope: "any URL" could apply strip-and-keep to non-git credential-bearing URLs Closed 24c74ea

Verdict

All previously identified findings are resolved. This commit is a clean, targeted fix: the exception is now bounded by its rationale rather than implied by context, and the non-generalization paragraph makes the boundary explicit without duplicating prose from the rest of the rule. Ready to merge.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

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


Security Review — PR #1780 (latest commit 24c74ea)

  • Gather context and understand the request
  • Get PR diff to see what the latest commit changed
  • Read changed files at HEAD
  • Perform security analysis
  • Post findings

What 24c74ea changes

One new commit: "fix(session-flow): scope the strip-and-keep exception to git remote URLs."

Two files changed: reference/save-point.md and CHANGELOG.md.

The prior commit (001808c) introduced the git-URL strip-and-keep exception with the phrase "any URL in the outbound set is checked for an @ ahead of its host." The last security review flagged this as a SUGGESTION: a model parsing that literally could apply the host-preserving strip to any credential-bearing URL — a database connection string like mongodb+srv://user:secret@host/db would survive with its host and path intact, disclosing infrastructure for no recovery benefit. This commit addresses that directly.


Assessment of 24c74ea

The fix is correct and closes the prior SUGGESTION

save-point.md lines 63–84:

The revised text now says "every git remote URL in the outbound set" (emphasis in the original) instead of "any URL." It adds an explicit negative-constraint paragraph:

The exception does not generalize to other credential-bearing URLs. A connection string such as mongodb+srv://<user>:<secret>@<host>/<db> keeps the general treatment — a shape marker (<REDACTED: database connection string>), not a host-preserving strip. What earns a git remote URL its exception is that something downstream re-resolves from the surviving host and path; nothing re-resolves from a database host, so preserving it discloses infrastructure for no recovery benefit.

This is the right fix and the right reasoning. The distinction between strip-and-keep (load-bearing remainder) vs. shape-marker (nothing downstream needs the value) is now stated explicitly rather than left to context inference.

The consumer side (find-handoff/SKILL.md) was already correctly scoped to "the Handoff origin: value" — not all URLs — and was not modified. No update needed there.


Prior findings — all resolved

Finding Resolution
Handoff origin: embedded raw credential-bearing git remote URLs Fixed a9ee2dc
Widening globbed under unverified cwd (home-directory sweep) Fixed a9ee2dc
Checklist satisfied without reading credential-stripping rule Fixed ae3a497
Two rules prescribed different outputs for same class, no precedence stated Fixed 001808c
Strip-and-keep exception scoped to "any URL" — applies to non-git credential URLs Fixed 24c74ea

No new security issues found

The commit is confined to the prose rule for the git-URL exception and the CHANGELOG. No logic changes, no new code paths, no new surfaces. The consumer-side rule (find-handoff/SKILL.md) was not modified and remains correctly scoped.

Ready to merge from a security standpoint.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification of the six re-review comments triggered by a9ee2dc4 / ae3a497f / 001808c9. Five report prior findings closed with no new issues (5131226336, 5131227923, 5131418046, 5131419346, 5131628312) — nothing to classify on those beyond confirming they raise no findings. One new finding, from 5131628811:

# Finding Classification Evidence Reacted
1 SUGGESTION · LOW — "any URL in the outbound set is checked for an @ ahead of its host" is broader than the git-remote-only exception it introduces; read literally, a connection string such as mongodb+srv://<user>:<secret>@<host>/<db> would take the host-preserving strip rather than a shape marker, leaving host and path visible where the general rule redacts them VALID — fixed in 24c74ea6 Confirmed: the sentence said "any URL", and while the paragraph's opening and <repo-identity>'s constraint both imply git remotes, the operative instruction did not say so. Agreed the credential is removed under either reading, so this is infrastructure disclosure rather than a credential leak — but it is text this PR introduces, and the fix is one clause, so there is no reason to carry it. Now reads "every git remote URL in the outbound set" and "it wins for git remote URLs and nothing else", plus an explicit non-generalization paragraph: a connection string keeps <REDACTED: database connection string>. The paragraph also states the DISCRIMINATOR rather than just the carve-out, since that is what makes the rule executable on a URL shape neither of us enumerated — strip-and-keep applies where the remainder is load-bearing, because recovery re-resolves from a remote's surviving host and path, and nothing re-resolves from a database host. Your note that the consumer side (find-handoff/SKILL.md) was already correctly scoped to "the Handoff origin: value" matches what I see; no change was needed there. 👍

Verified: markdownlint-cli2 on both changed files → 0 errors.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Per-finding dispositions for the restatements carried by the re-review rounds. Each row below is a finding a later review round re-listed while confirming it closed; recording the disposition explicitly rather than leaving it implied in the originating thread, since a restatement in an open comment is its own work item.

# Finding Classification Evidence Reacted
1 Re-review 5131226336 re-lists the credential-bearing remote URL embedded in Handoff origin: VALID — already fixed in a9ee2dc4 save-point.md <repo-identity> strips the credential-bearing userinfo before embedding and falls back to the root directory name when the boundary is undeterminable; both redaction passes name remote URLs as a vector; handoff eval 5 asserts it. That review's own status table records it Fixed. 👍
2 Re-review 5131226336 re-lists the widening globbing under an unverified transcript cwd VALID — already fixed in a9ee2dc4 The widening admits a cwd only after git -C <cwd> rev-parse --show-toplevel confirms it, globs under that top level, and contributes no root when there is none; find-handoff eval 12 covers it and eval 10's stale expectation was corrected. That review's status table records it Fixed. 👍
3 Re-review 5131628811 re-lists the producer checklist being satisfiable without reading the stripping rule VALID — already fixed in ae3a497f handoff/SKILL.md's resume-prompt checklist item now names the requirement inline as a qualifier on the Handoff origin: clause, so the rule is visible where the box is ticked. That review's status table records it Fixed. 👍
4 Re-reviews 5131760407 and 5131761328 re-list the strip-and-keep exception's scope wording VALID — already fixed in 24c74ea6 The rule reads "every git remote URL in the outbound set" and "it wins for git remote URLs and nothing else", with an explicit non-generalization paragraph keeping the shape marker for a connection string and stating the discriminator (strip-and-keep applies where the remainder is load-bearing for recovery). Both of those reviews confirm the wording closes it and raise no new issues. 👍

No open findings remain on this PR. Final head 24c74ea6; all 31 checks green; 0 unresolved threads.

@kyle-sexton
kyle-sexton merged commit eb1c243 into main Jul 30, 2026
31 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1644-rooted-resume-prompt-path branch July 30, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant