Skip to content

fix(education): take the illustrate slug from the model, not the build command - #7807

Merged
kyle-sexton merged 6 commits into
mainfrom
work-resolve-issue/issue-7374-39058663258-1
Oct 17, 2026
Merged

kyle-sexton merged 6 commits into
mainfrom
work-resolve-issue/issue-7374-39058663258-1

Conversation

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

Closes #7374

Hold: a human merges this

Summary

Opened by the work-resolve-issue lane from work-resolve-issue/issue-7374-39058663258-1, which an earlier run left with commits and no report. The model did not run again, and the earlier run's outcome could not be read, so this body carries none of its text.

Fix

The diff shows the changes.

Verification

The earlier run reported no checks here.

Left out: every check; a person reviews the diff before merging.

Related

#7374

…command

The builder takes --out-dir, reads the slug from the model JSON, rejects one
outside ^[a-z0-9-]+$ with exit 2, and derives the record and page paths itself.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review needs no new review of 1712f0e766ef8f1e8098b2eb8b29a8eb750addfd: no file of this pull request changed since the review of 37420a4.

No security issues in the incremental change since e617d36 (reviewed head 37420a4).

The change moves the illustrate model file from the shared model.json to a per-session models/${CLAUDE_SESSION_ID}.json. It touches the changelog fragment, SKILL.md and build-explainer.test.sh.

  • Trust boundary: the build command still carries only fixed paths and the session ID, which the harness sets. No model-chosen or user-chosen text reaches the command line. The builder still checks the slug against ^[a-z0-9-]+$ and derives the record and page paths itself.
  • Shared-directory race: per-session files remove the cross-session overwrite window. This strengthens the control, and the "read it before you write it" step and the race caveat are gone because they are no longer needed.
  • Path injection: CLAUDE_SESSION_ID is a harness-generated UUID, not attacker-controlled, so it gives no traversal path.
  • Test change: the test now also asserts that the build command uses a per-session model file. The edit adds a check and weakens none.
  • Instruction surface: the removed text is the old shared-file read-before-write and race workaround. The per-session path replaces it, so no guardrail is left unenforced.

Only the three listed files were reviewed. I did not consult the instruction exception register, because the diff removes no standing guardrail.

Comment thread plugins/education/skills/illustrate/SKILL.md Outdated
@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

claude-review needs no new review of 1712f0e766ef8f1e8098b2eb8b29a8eb750addfd: no file of this pull request changed since the review of 37420a4.

No findings. I reviewed the incremental diff since e617d36 in three files: the changelog fragment, plugins/education/skills/illustrate/SKILL.md and build-explainer.test.sh. I did not run the test script or the builder.

  • Skill change: SKILL.md now has the model written to illustrate/models/${CLAUDE_SESSION_ID}.json, so each session gets its own file. The old "Read it before you write it" and "run straight after the write" caveats are removed, which fits because two sessions no longer share the file. The fixed build command keeps carrying only fixed paths, and grep of plugins/education finds no leftover illustrate/model.json reference.
  • Test change: Pulling buildCommand into a variable is a clean refactor. The new check pins --model "...${CLAUDE_SESSION_ID}..." in the first bash block, which is the build command. It also carries a portability-ok marker like the neighbouring checks.
  • Changelog: The new sentence in the fragment matches the skill change, and the bump level (patch) is unchanged.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

pr-unblock / address-feedback finished round 1 of 3 on b0fa036.

Comment thread plugins/education/skills/illustrate/SKILL.md Outdated
…ite tool behavior

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

pr-unblock / address-feedback finished round 2 of 3 on e617d36.

The fixed illustrate/model.json was shared by every session using the
data directory, so one session could build another's topic. Write the
model to models/${CLAUDE_SESSION_ID}.json instead, drop the sentences
that restated the Write tool's Read-first behavior, and add a test that
the skill's build command reads a per-session model file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YcPjcqFavvzkTGBKXMgdXR
@kyle-sexton kyle-sexton changed the title chore: work on #7374 fix(education): take the illustrate slug from the model, not the build command Oct 17, 2026
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 17, 2026
Merged via the queue into main with commit 0cdd9df Oct 17, 2026
29 checks passed
@kyle-sexton
kyle-sexton deleted the work-resolve-issue/issue-7374-39058663258-1 branch October 17, 2026 22:56
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

Unattended merge: risk MED at head 1712f0e766ef8f1e8098b2eb8b29a8eb750addfd. Only LOW merges without a human; this pull request waits for one.

MED evidence-unbound:claude-review: not bound: no-run
MED evidence-unbound:claude-security-review: not bound: no-run

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.

fix(education): keep the LLM-chosen slug out of illustrate's build command

1 participant