Skip to content

fix(review): harden explain-change overlay containment, risk_paths globbing and stale references #6110

Description

@kyle-sexton

Context

Found in the reviews of #6018 (feat(review): add review:explain-change with a digest policy and an interactive view) and #6013 (feat(education)!: replace eli5 with illustrate). Found while delivering spec container #5835.

Problem

From #6018:

  1. The digest overlay has no realpath containment check. A symlinked .claude is rejected today only incidentally.
  2. risk_paths glob matching can backtrack catastrophically on a pattern from a trusted layer.
  3. records.json rulings still name review:pr-explainer.

From #6013:
4. The SKILL.md heredoc could pass the model through a scratch file instead.
5. Where the explainer-video output lands relative to the record is owned by that plugin and should be confirmed there.

Suggested fix

  1. Resolve the overlay path with realpath and require it to stay inside the repo before reading.
  2. Bound or linearize the glob matcher (no nested quantifier expansion) and cap pattern length.
  3. Update the records.json rulings to the current skill name.
  4. Write the model to a scratch file and pass the path.
  5. Confirm and document the output location with the explainer-video owner.

Related: #5835

Activity

  1. kyle-sexton commented on Oct 11, 2026

    @kyle-sexton
    ContributorAuthor

    Draft PR: #7191 (branch fix/6110-explain-change-hardening).

    • Item 2: risk_paths globs now go through a linear matcher with caps of 64 globs and 256 characters. Before, a star-heavy glob hung the regex even on a 30-character path; now a 5000-character path finishes in under 2 s.
    • Items 4 and 5: illustrate passes the model by --model <file> instead of a heredoc, and the video offer leaves the output location to /explainer-video:produce.
    • Items 1 and 3 were already fixed on main: the overlay realpath check and its tests are in lib/publish-gate.mjs, and records.json no longer names pr-explainer.
    • The explain-change tests (117 pass, 1 skipped for no Chrome), the illustrate builder tests and the changed-skills gate all pass.

    Next step: run scripts/affected-tests.sh --base origin/main --run before marking the PR ready. The session wrapped up before that full sweep.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority: lowNice-to-have, cosmetic, or speculative; opportunistic.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions