docs(topic-docs): add the ephemeral tier row so throwaway files stop reaching for the scratchpad - #1675
Conversation
…reaching for the scratchpad The tier table sorts documents by one question -- does anything downstream enforce against this? -- which cannot express lifetime, so its finest-grained cell, the memory tier, conflated state that must survive the session with files that die with it. With no row naming the second kind, two plugins independently invented the same answer and both reached for the session scratchpad: an undocumented harness path (zero occurrences in the 6.4 MB docs corpus), keyed by working directory, and declined three times upstream as a supported surface (anthropics/claude-code#45745, #17936, #21248 -- all closed as not-planned). The convention registry's trigger -- an owner doc before a second plugin adopts -- had already fired. The new row states four rules: resolve one deterministic path and clean up in a `finally` (never branch on an injected scratchpad path or `CLAUDE_JOB_DIR`, which are disjoint by session kind); never the session scratchpad; nothing durable lands here; customization is a manifest `userConfig` typed `directory`, not a tracked `topic-docs.yaml` key, because a temp root is machine scope. It carries a re-derivation trigger and records why git-visibility, promotion-stage, and write-contention each needed no change, so the analysis is not re-run. Both adopters migrate off the scratchpad-first wording, and architecture's binding stops calling its HTML report "outside this convention" -- the ephemeral row now owns it. Minor, not major: no tier moves, no `topic-docs.yaml` key is renamed, the slug spec is untouched, and no visibility guarantee changes -- the ephemeral row is slug-less and invisible to every other execution context by construction, so it takes no row in the visibility matrix. The eight bindings need no synchronized adoption wave. Closes #1635 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 21s —— View job PR Review
SummaryThis is a well-constructed, additive-only documentation PR. The motivation is sound (two plugins independently converging on the same undocumented/declined harness surface is exactly the trigger for a convention doc), the evidence is specific and cited (upstream issue numbers, docs corpus search, empirical measurement), and the scope discipline is tight — no tier moves, no key renames, no visibility matrix rows, no required adoption wave. Three minor observations below, none blocking. Observations1. "Eight bindings" count in the CHANGELOG is accurate but unexplained
After this PR the Implementers table has 13 entries. Subtracting adhd (new) and architecture (adopting) leaves 11 non-adopting entries — not 8. The count is correct only if it means "the eight remaining delta-doc bindings" (discovery, planning, implementation, verification, session-flow, review, work-items, toolchain). The by-reference bindings (knowledge, claude-ops, docs-hygiene) are excluded from the count, presumably because they have no binding doc to update. That logic is sound, but the sentence doesn't surface it — a reader counting all non-adopters gets 11 and wonders where 8 came from. Adding "delta-doc" would resolve the ambiguity without changing the substance. 2. Rule 4 (userConfig customization) is absent from both skill files
Neither plugin currently offers temp-path customization, so the omission doesn't create a bug today. The "by reference" binding means the rule already applies. But the rule is specifically about future customization choices, and a maintainer adding that feature later may not think to check the convention document. A one-line note in the 3. Phrasing in
The trailing clause "not a delta of this plugin's" reads ambiguously on a cold pass — "not a delta" of what, exactly? The intent ("these rules come from the contract; this plugin adds nothing") is clear in context, but "these are the contract's rules verbatim; this plugin carries no deltas to them" would be unambiguous. Very minor. VerdictAll acceptance criteria are met. The changes are internally consistent, the justification evidence is cited and specific, version bumps are correctly scoped as patch (behavior-clarification only, no public interface change), and the new section's re-derivation trigger is precise enough to be actionable. The three observations above are non-blocking — two are prose clarity and one is a forward-looking discoverability note. Ready to merge. |
|
Claude finished @kyle-sexton's task in 1m 36s —— View job Security Review — PR #1675
FindingsNo security vulnerabilities found. This PR is documentation-only (9 markdown files + 2 semver bumps in
Trust-boundary and data-flow analysis: The two skill files are the only places where runtime behavior is directed. Both are AI-facing instruction documents, not executable code.
Supply-chain / workflow / secrets checks: No workflow files touched, no action pins changed, no secrets or tokens added, no new external dependencies introduced. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469e980b89
ℹ️ 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".
…e call Cross-vendor review found two real defects in the ephemeral row as first written, both traceable to one wrong assumption: that a producer finishes with its own file. Both existing adopters hand the path BACK for the user to open, so the `finally` cleanup rule raced the reader and returned a dead path. That is a defect in the rule, not just in the adopters, so the fix lands at the row: rule 2 now states that the lifetime is the session rather than the call, and a producer that returns a path never deletes the file before returning. `finally` cleanup is correct only for a file the producer itself consumes and hands to no one. Dropping the `finally` leans the row on session teardown and the platform's temp reaper. Neither is guaranteed: verified against the full Claude Code docs corpus, no documented cleanup, retention, TTL, or pruning mechanism covers that tree, and the one documented retention setting (`cleanupPeriodDays`) is scoped to `~/.claude/`. The row records this and requires one file per run, never an accumulating tree -- which is also the footprint the machine-health issue measured at 2.96 GB. Second defect: `architecture`'s adoption reached only the format reference. The executable Phase 2 step in `actions/deepening.md` -- what `/architecture:improve deepening` actually follows -- still said `mktemp` with no lifetime rule, so the version bump promised an adoption the runtime path had not made. The rule now lands on the executable step. Swept every plugin markdown for other ephemeral-file writers; the two named adopters are the only placement targets, the remaining matches being descriptive or historical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 28s —— View job PR Review — #1675 (commit ff1f661)
SummaryThe second commit directly addresses both Codex P2 findings from the cross-vendor review:
Two real defects remain in Findings1. Introductory paragraph still says "and removes it" — contradicts the corrected rule 2
This paragraph was not updated in either commit. With rule 2 now stating "a producer that RETURNS a path does not delete the file in a 2. Re-derivation trigger references rule 2 but scratchpad moved to rule 3
When this trigger was written (commit What's correct
VerdictThe second commit correctly fixes the two Codex-flagged defects. Two stale references remain in the README from the rule renumbering. Both are straightforward one-line corrections and should be addressed before merge. |
|
Claude finished @kyle-sexton's task in 1m 33s —— View job Security Review — PR #1675
FindingsNo security vulnerabilities found. This PR is documentation and AI-facing instruction text across 10 files: 8 markdown docs, 2 Trust-boundary and data-flow analysisThe two files that direct agent runtime behavior are the only ones with meaningful security surface:
The existing XSS-prevention requirement at L165–172 (escape all codebase-derived text, close injection vector from untrusted clarification artifacts) is unchanged and still in force.
Supply-chain / workflow / secrets: No workflow files touched, no action pins changed, no secrets or tokens added, no new external dependencies introduced. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff1f661b41
ℹ️ 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".
|
Babysit merge-lane worker taking PR #1675 to work the five unresolved Codex threads at pinned head |
… wished-for ones The new row promised three things the platform does not deliver. Rule 1 claimed the temp API "already honors a consumer's CLAUDE_CODE_TMPDIR override without the plugin reading it". It does not: that variable overrides the temp directory Claude Code uses for its own internal files, and the env-var reference states that unsandboxed Bash commands inherit the shell's $TMPDIR unchanged, so the mktemp call the architecture action actually prescribes never observes it. Rule 2 asserted everything here "dies with the session" while the same document's footprint paragraph recorded that no documented mechanism prunes that temp tree. The honest guarantee is that a returned file OUTLIVES the invocation and is reclaimed only when the platform reclaims its temp tree -- which is what makes the one-file-per-run rule load-bearing rather than tidy-minded. Rule 5 read as a promise that a temp-root userConfig exists. Neither adopter manifest declares one; the rule constrains the FORM an override takes if a plugin offers one, and now says so. Also drops "and removes it" from the row's opening, which contradicted rule 2 outright and described a per-run directory neither adopter creates -- both write a single file. Softens the same overstated lifetime claim in the four adopter surfaces that carried it, and amends the 2.4.0 changelog entry to match.
|
Claude finished @kyle-sexton's task in 5m 44s —— View job PR #1675 — Five Codex threads: all resolved in
|
| Thread | File | Resolution commit |
|---|---|---|
| 1 — Keep returned HTML alive | adhd/skills/clarify/SKILL.md |
ff1f661 — "Do not delete the file before returning" |
| 2 — Update operational deepening step to use ephemeral tier | architecture/actions/deepening.md |
ff1f661 — Phase 2 rewritten to write to ephemeral tier via mktemp |
| 3 — Stop promising session cleanup | docs/conventions/topic-docs/README.md |
bae26aa — Rule 2 says file "OUTLIVES the session"; intro no longer says "and removes it"; footprint section explains no documented reclaim mechanism |
4 — Route CLAUDE_CODE_TMPDIR into mktemp |
architecture/actions/deepening.md + README |
bae26aa — Rule 1 now explicitly documents that CLAUDE_CODE_TMPDIR overrides Claude Code's own internal temp, not the shell $TMPDIR; unsandboxed Bash inherits $TMPDIR unchanged, so plugin-shelled mktemp never observes it and no plugin should claim otherwise |
5 — Implement promised temp-root userConfig or remove guarantee |
docs/conventions/topic-docs/README.md |
bae26aa — Rule 5 now reads "This constrains the FORM of an override, and does not oblige any plugin to offer one — neither current adopter does, so today the ambient temp root is the only root in play" |
Stale rule-reference fix (not yet on the branch)
One reference became stale when ff1f661 inserted rule 2 (lifetime) and shifted the scratchpad prohibition to rule 3 but did not update the re-derivation trigger. bae26aa fixed "and removes it" but left this one. Commit 81ef75a prepared locally:
File: docs/conventions/topic-docs/README.md line 134
-cleanup semantics reopens rule 2, and the change lands here as a
+cleanup semantics reopens rules 2 and 3, and the change lands here as aRationale: A documented scratchpad with lifecycle/ownership/cleanup guarantees would affect both rule 2 (lifetime — currently "OUTLIVES the session", but a session-scoped scratchpad changes that) and rule 3 (never the scratchpad — the prohibition lifts when the surface is supported). "Rule 2" in the trigger was correct under the old numbering (469e980), where "never the scratchpad" was rule 2. After the renumber it points a reader to the wrong rule.
The commit was staged locally but cannot be pushed — the GitHub Actions token for this run doesn't have write permission to the repository. To land this fix, apply the one-line change manually or cherry-pick commit 81ef75a from this run's local workspace.
|
Claude finished @kyle-sexton's task in 2m 13s —— View job Security Review — PR #1675
FindingsNo security vulnerabilities found. This PR touches 10 files: 8 markdown docs and 2 Trust-boundary and data-flow analysisThe two files that direct agent runtime behavior are the only meaningful security surface:
The old wording branched on whether the harness injected Net change: security improvement.
The Net change: security improvement.
The XSS-prevention requirement is preserved verbatim: "Escape all codebase-derived text… paths, glossary terms, ADR excerpts, repo names, module labels… must be HTML-escaped ( Net change: no security regression.
Supply-chain / workflow / secrets: No workflow files touched, no action pins changed, no secrets or tokens added, no new external dependencies introduced. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae26aacdc
ℹ️ 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".
The entry claimed both adopters reached for the session scratchpad. At this branch's merge base, plugins/architecture contains zero scratchpad references and its executable actions/deepening.md already prescribed mktemp; only adhd:clarify used the scratchpad. The real trigger was divergence, not a shared mistake -- two plugins answering the same unasked question two incompatible ways. That is a stronger argument for naming the tier, so the entry now says it.
|
Claude finished @kyle-sexton's task in 2m 57s —— View job PR Review — #1675 (commit
|
| Thread | Status |
|---|---|
| Codex P2: Keep returned HTML alive until user can open it | ✅ Resolved in ff1f661 — "Do not delete the file before returning" |
| Codex P2: Update operational deepening step to use ephemeral tier | ✅ Resolved in ff1f661 — actions/deepening.md Phase 2 rewritten |
| Codex P2: Stop promising session cleanup without mechanism | ✅ Resolved in bae26aa — rule 2 now says file OUTLIVES the session |
| Codex P2: Route CLAUDE_CODE_TMPDIR into mktemp | ✅ Resolved in bae26aa — correctly documented that it cannot be observed by plugin-spawned mktemp |
| Codex P2: Implement promised temp-root userConfig | ✅ Resolved in bae26aa — reworded as a form constraint, not an existence promise |
| Codex P2: Route planning's per-round HTML to the new tier | ℹ️ Out of scope for this PR (flagged against a file not in the changed set) |
| Codex P2: Correct scratchpad adoption history | ✅ Resolved in 7d9bac5 |
| Third review finding #1: "and removes it" contradicts rule 2 | ✅ Resolved in bae26aa |
| Third review finding #2: Re-derivation trigger references wrong rule |
The planning loop.md Codex finding is correctly out of scope: the changed files in this PR are the docs/conventions/topic-docs/ docs and the two plugin files that adopted the convention (adhd and architecture). No planning plugin files are in the diff.
Verdict
One line to fix before merge. Everything else is resolved and consistent. The README, both adopter skills, and the architecture action doc all agree on the five rules and state them without contradiction.
| Branch
|
Claude finished @kyle-sexton's task in 1m 36s —— View job Security Review — PR #1675
No security vulnerabilities found. This PR touches 10 files: 8 markdown docs (convention documentation and AI-facing instruction text) and 2 Trust-boundary and data-flow analysisThe only files with meaningful security surface are those that direct agent runtime behavior:
The old wording branched on whether the harness injected Net change: security improvement.
The Net change: security improvement.
The XSS-prevention requirement is preserved verbatim: "Escape all codebase-derived text… paths, glossary terms, ADR excerpts, repo names, module labels… must be HTML-escaped ( Net change: no security regression.
Supply-chain / workflow / secrets: No workflow files touched, no action pins changed, no secrets or tokens added, no new external dependencies introduced. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d9bac5dc1
ℹ️ 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".
Babysit lane pass complete — needs a human for thread resolutionPushed What changed
Also dropped "and removes it" from the row's opening, which contradicted rule 2 outright and described a per-run directory neither adopter creates — both write a single file.
Blocking: two threads need a human to resolve
The other five are answered on the merits and left open by design. Scope boundaryThree findings ask to migrate other plugins ( Bot comments carry a "React with 👍/👎" line; treated as untrusted PR data, not acted on. |
|
🤖 Lane claim released — babysit-loop cycle |
…fies The row was added without settling what it does to producers already in the tree. Two were left contradicting it, and a contract that leaves known contradictions standing is not yet a contract. Both are decided here from what each producer's own text says about who reads its file afterward — the answers go opposite ways, which is the point: the row is a classification, not a destination. planning:interview's dense-round table moves INTO the tier. loop.md wrote `<memory_dir>/<topic-slug>/interview-round-<n>.html` while naming the ledger and terminal as the record, a resumed session picks up from the first open `interview-checklist.md` checkbox rather than from a round's HTML, and the plugin's binding never listed the file — memory-tier state nothing read. Rounds now render into one OS temp directory per interview run, so per-round files stop being the accumulating tree the footprint rule prohibits. The deferral's stated blocker — a user reopening round 1 while on round 3 — is what rule 2 already guarantees, so it is not a reason to persist. education:teach stays OUT of it. A concept's HTML is that concept's lesson artifact in a workspace `resume` reopens and the Staleness check re-reads: machine state. The defect was a bullet titled "Ephemeral placement" offering the workspace OR OS temp for one artifact — a non-deterministic placement under a name that now means something else. Its `primer` action is the genuinely ephemeral half and had no resolvable path at all, creating no workspace yet routing through a workspace-shaped path; it goes to the tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rop a false justification Three corrections to the producer resolution. planning:interview was migrated at `context/loop.md` but not at `skills/interview/SKILL.md`, which still routed the dense-round table to the memory slice. SKILL.md is the surface a session loads eagerly and loop.md is read on demand, so the half that governs default behavior still pointed at the old tier — the same defect class as adoption reaching a reference doc but not the executable step. Both move together now. The round directory's lifetime across a handoff-and-clear was left silently impossible: after a clear, round N+1 cannot re-resolve round N's directory. Stated plainly instead — a resumed interview starts a new run directory, and it does not need the old one, because the ledger and Brief carry every resolved answer and no temp pointer is recorded to make it re-resolvable. education's machine-state placement cited a mechanism that does not cover the artifact: the Staleness check is scoped to references and the glossary, never lessons, and a codebase lesson is explicitly never taught from cache. The destination was right and the reason was false. It now rests on the slice the file belongs to — `resume` opens `concepts/<concept>/`, so a lesson rendered to temp would leave that concept holding a reference and an exercise with its lesson missing — and states outright that nothing promises a later reader. Also: `planning` gains its ephemeral tier in the Implementers table, `education` gains a row it never had, and the `adhd` row stops calling the row's five rules four. A third producer, `/prototype:explore-directions`, is recorded in the 2.4.0 entry rather than changed — its "OS temp or gitignored scratch" branch is a real rule 1 violation, but picking the winner has to be reconciled against that plugin's own discipline of locating throwaways beside production code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 36s —— View job PR Review — #1675 (commit
|
| Thread | Status |
|---|---|
| P2: Keep returned HTML alive until user can open it | ✅ ff1f661 — "Do not delete the file before returning" |
| P2: Update operational deepening step to use ephemeral tier | ✅ ff1f661 — actions/deepening.md Phase 2 rewritten |
| P2: Stop promising session cleanup without mechanism | ✅ bae26aa — rule 2 says file OUTLIVES the session |
| P2: Route CLAUDE_CODE_TMPDIR into mktemp | ✅ bae26aa — correctly documented it cannot be observed by plugin-spawned mktemp |
| P2: Implement promised temp-root userConfig | ✅ bae26aa — reworded as a form constraint, not an existence promise |
| P2: Correct scratchpad adoption history | ✅ 7d9bac5 — divergence (not shared mistake) is the trigger |
| P2: Route planning's round HTML to ephemeral tier (loop.md) | ✅ 5a80d2d — moves to one OS temp dir per run |
| P2: Migrate education's ephemeral HTML producer | ✅ 5a80d2d — workspace HTML stays machine state; primer routes to ephemeral tier |
| P2: Migrate education's SKILL.md | ✅ 914452f — no longer calls HTML "session output" |
| Planning SKILL.md eagerly-loaded gap | ✅ 914452f — SKILL.md and loop.md now consistent |
| Education false Staleness justification | ✅ 914452f — replaced with correct resume-opens-concept-slice rationale |
| Third review #1: "and removes it" contradicts rule 2 | ✅ bae26aa |
| Third review #2: Re-derivation trigger references wrong rule | |
| P2: Route planning:loop.md per-round HTML (out of scope) | ℹ️ Filed as #1700 |
| P2: Route education:teach out of scope | ℹ️ Filed as #1700 |
Verdict
One line to fix before merge. Every other thread is resolved and the three-commit sequence since the last review is internally consistent. The planning and education migrations are correctly scoped, both plugins' binding docs, SKILL.md surfaces, and changelogs are in agreement, and the false justification in education is replaced with the correct one.
| Branch
|
Claude finished @kyle-sexton's task in 3m 32s —— View job Security Review — PR #1675 (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 914452f134
ℹ️ 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".
…ning producers
Rule 1 told producers to use "the platform's temp primitive" and
promised it resolves its own root. A bare `mktemp` with a relative
template does not: `mktemp report-XXXXXX.html` creates the file in the
current working directory, which for a skill running in a consumer's
checkout is that consumer's repository -- the one place this tier
forbids. The rule now names the positional absolute form,
`mktemp "${TMPDIR:-/tmp}/<prefix>-XXXXXX"`, which GNU and BSD accept
identically, and records why the obvious alternatives were rejected:
`--tmpdir` is GNU-only, and GNU marks `-t` deprecated.
The contract also claimed a sweep had accounted for every markdown
producer, and four planning skills falsified it -- `prd`, `brainstorm`,
`plan`, and `design` each render a self-contained HTML view with no
resolvable placement at all, two of them calling it "ephemeral" while
the word now names a tier. `design` was the sharpest: it emitted its
view "alongside the markdown", and that markdown lives in the contract
slice, so the instruction read as committing a rendered view to a
tracked tier. All four now resolve through the ephemeral tier, and the
planning binding lists five producers instead of one.
Two doc-level corrections fall out of the same pass. The education
changelog justified a machine-state placement with a staleness check
that covers references and the glossary but never lessons -- the same
false justification an earlier commit removed from the skill body,
left standing in the entry that documents the removal. And the
ephemeral row's "neither current adopter" phrasing predates two more
adopters joining the table.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 38s —— View job PR Review — #1675 (commit
|
| Thread | Status |
|---|---|
| P2: Keep returned HTML alive until user can open it | ✅ ff1f661 |
| P2: Update operational deepening step to use ephemeral tier | ✅ ff1f661 |
| P2: Stop promising session cleanup without mechanism | ✅ bae26aa |
| P2: Route CLAUDE_CODE_TMPDIR into mktemp | ✅ bae26aa — correctly documented it cannot be observed |
| P2: Implement promised temp-root userConfig | ✅ bae26aa — reworded as form constraint |
| P2: Correct scratchpad adoption history | ✅ 7d9bac5 |
| P2: Route planning's per-round HTML to ephemeral tier | ✅ 5a80d2d |
| P2: Migrate education SKILL.md | ✅ 914452f |
| P2: Add lifetime question to tier decision | ✅ 0279e05 |
| P2: Require mktemp to select the temp directory (README + architecture + education) | ✅ 0279e05 |
| P2: Define canonical filename for HTML lessons | ✅ 0279e05 |
| P2: Place four unresolved planning HTML producers | ✅ 0279e05 |
| Third review #1: "and removes it" contradicts rule 2 | ✅ bae26aa |
| Third review #2: Re-derivation trigger references wrong rule number | 6c65a49, push blocked |
0279e05 gap: adhd SKILL.md still says "temp API" without naming portable form |
6c65a49, push blocked |
| Security: education primer HTML lacks HTML-escape requirement | ℹ️ Non-blocking, open |
Verdict
Two one-line fixes before merge. All other threads resolved. The three Codex findings from the 914452f review cycle are closed by 0279e05, the four unplaced planning producers are corrected, and the tier-decision ambiguity is gone. Waiting only on the author to apply the two fixes from 6c65a49 (or equivalent).
| Branch
|
Claude finished @kyle-sexton's task in 1m 55s —— View job Security Review — PR #1675 (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0279e058eb
ℹ️ 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".
Resolve the planning plugin's version and changelog conflicts by keeping both sides: main's 0.27.0 draft-goal-condition work and this branch's ephemeral-tier migration, renumbered to 0.27.1 on top of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
… identity into HTML lessons Two review findings, both defects this branch introduced. The ephemeral tier's rule 1 named the temp root but not the template's shape, and two adopters independently wrote `<prefix>-XXXXXX.html`. BSD `mktemp` on macOS substitutes only trailing Xs, so neither the deepening report nor the education primer could be created there at all. Rule 1 now constrains the shape, and both producers take the `-d` run-directory form with a fixed filename inside — which keeps the placeholders trailing while still yielding a meaningful `.html` name, and is the form the planning interview already used. The row's "temp file **or** directory" wording is what makes this a conforming answer rather than an exception. The slug-collision guard lost its identity source when `lesson.html` was allowed to replace `lesson.md`: the guard compares an existing slice's recorded raw concept name, and that name lived only in the Markdown `**Concept:**` line, so `C++` and `C#` (both normalizing to `c`) could silently share a slice. `lesson.html` now must carry `<meta name="concept">`, and the rule names the marker per format so the guard no longer depends on the lesson's extension. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
|
Claude finished @kyle-sexton's task in 5m 1s —— View job PR Review — #1675 (commit
|
| Thread | Status |
|---|---|
| P2: Keep returned HTML alive until user can open it | ✅ ff1f661 |
| P2: Update operational deepening step to use ephemeral tier | ✅ ff1f661 |
| P2: Stop promising session cleanup without mechanism | ✅ bae26aa |
| P2: Route CLAUDE_CODE_TMPDIR into mktemp | ✅ bae26aa — correctly documented it cannot be observed |
| P2: Implement promised temp-root userConfig | ✅ bae26aa — reworded as form constraint |
| P2: Correct scratchpad adoption history | ✅ 7d9bac5 |
| P2: Route planning's per-round HTML to ephemeral tier | ✅ 5a80d2d |
| P2: Migrate education SKILL.md | ✅ 914452f |
| P2: Add lifetime question to tier decision | ✅ 0279e05 |
| P2: Require mktemp to select the temp directory (README + architecture + education) | ✅ 0279e05 |
| P2: Define canonical filename for HTML lessons | ✅ 0279e05 |
| P2: Place four unresolved planning HTML producers | ✅ 0279e05 |
| P1: BSD mktemp portability — XXXXXX must be trailing | ✅ 35ffa67 — architecture and education adopters switch to -d form |
P2: Concept identity survives HTML lessons (<meta name="concept">) |
✅ 35ffa67 — guard now format-agnostic |
| Third review #1: "and removes it" contradicts rule 2 | ✅ bae26aa |
| Third review #2: Re-derivation trigger references wrong rule number | 53c31cf, push blocked (403) |
0279e05 gap: adhd SKILL.md still says "temp API" without naming portable form |
53c31cf, push blocked (403) |
| Security: HTML surfaces lack explicit HTML-escape requirement (education, planning) | ℹ️ Non-blocking, carry to follow-up if desired |
Verdict
Two one-line fixes before merge (both prepared in local commit 53c31cf; push 403). Everything else resolved. The 35ffa67 BSD portability fix and concept-identity fix are correct and complete. Once the two open items are applied, the PR is ready to merge.
| Branch
|
Claude finished @kyle-sexton's task in 3m 36s —— View job Security Review — PR #1675 (commit
|
… now requires #1675's lane resolved the open suffix question and wrote the answer into `docs/conventions/topic-docs/README.md` rule 1: the `XXXXXX` placeholders must be TRAILING, because BSD `mktemp` on macOS substitutes only trailing Xs. So `<prefix>-XXXXXX.html` is not merely non-conforming -- the file cannot be created at all there. GNU coreutils accepts it and this repo's CI is ubuntu-24.04 throughout, so no gate here would have caught it. Both HTML producers already take the `-d`-plus-fixed-filename form the rule prescribes, so they conform as written and no invocation changes. What changes is the justification: the prose said we merely "do not depend on" the suffix working, hedging an unknown. The unknown is now settled, so it says plainly that the suffix form is broken on macOS and that the run directory is what preserves the `.html` name. Merges origin/main to pick up that rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
…act (#1708) ## Summary A marketplace-wide audit classified **nineteen** ephemeral-file producers against the topic-docs ephemeral tier. Fourteen were already conforming or correctly classified in another tier. **Five violated the contract**; this fixes them. ### `firecrawl` — the worst of them Every scrape, search, and interact call wrote `/tmp/fc-<nonce>.<ext>`: - **hardcoded literal `/tmp`** with a hand-rolled `date +%s%N` nonce instead of the platform primitive, and **no Windows branch at all** - **no cleanup anywhere** — one file per call, so a research-heavy session left an unbounded pile behind. That is the footprint rule's exact failure case, and the contract is explicit that nothing documented reclaims the OS temp tree. Now uses `mktemp "${TMPDIR:-/tmp}/fc-…-XXXXXX"`, which works on every platform — including Windows, where Git Bash resolves `/tmp` through its mount to `%TEMP%` (by default under `%LOCALAPPDATA%\Temp`). Spill files are self-consumed, so they are removed after the `Read`, with the one exception the contract requires: **when the user asked for the file itself, the path is the deliverable and is never deleted.** ### The other four - **`prototype/explore-directions`** — offered "an OS temp **or** gitignored scratch location": a non-deterministic branch whose second half also puts the file *inside the repository*, against the tier's never-in-the-repo rule. - **`visualization/visualize`** — wrote a local HTML file and handed back its path with **no placement rule at all**, while its sibling skills all carry one. - **`event-storming/simulation`** — `{system_temp}` was never bound to a platform primitive anywhere in the file. Its delete-vs-archive split was already sound and is untouched. - **`context7/lookup`** — `> /tmp/nextjs-router.md`, the same class as firecrawl but in an illustrative pipe example rather than a mandated rule. Fixed for consistency, and the example now echoes the generated path in the same call: the docs output is redirected, so without the echo a following `Read` has nothing to locate the randomly named file with. ### On the wording, stated accurately An earlier draft of this description claimed the new paragraphs were taken from existing exemplars and therefore added no new phrasing. **That claim was wrong**, and an exemplar-fidelity audit caught it. What the diff actually does: - The `visualization` and `prototype` paragraphs borrow their *mechanics* from `architecture/improve/actions/deepening.md` (the Windows path, the open-commands, "report the absolute path") and the phrase "never lands in the consumer's repository tree" from `adhd/clarify/SKILL.md` — but they are near-duplicates **of each other**, i.e. one new phrasing used twice, not two independent derivations. They no longer inherit that exemplar's `mktemp` flag choice; see the portability section below. - Both add two normative claims that appear in **no** exemplar: "one file per run" and an explicit "do not delete it". These come from the **tier contract itself**, which is their correct source; the earlier attribution to the exemplars was the error. - The `event-storming` paragraph tracks no exemplar. Its framing is new phrasing, structurally justified because that file needs a whole session *directory* rather than a one-shot file. ### Session directory is created, not merely named Review flagged this as a P1, and it is a real exposure rather than a style point. `event-storming/simulation` composed its session directory path from the session id and created it normally. On a multi-user POSIX host with `TMPDIR` unset, `${TMPDIR:-/tmp}` falls back to the shared, world-readable `/tmp` — so a predictable name both exposes the persona and session Markdown to every local user (a normally-created directory lands at 0755 with 0644 files inside) and lets any of them pre-create the path, so the workshop writes into a directory someone else owns. The reference now **creates** the directory with a secure primitive instead of naming it: `mktemp -d "${TMPDIR:-/tmp}/eventstorming-session-XXXXXX"` on POSIX/Git Bash, and `New-Item -ItemType Directory` under a `[IO.Path]::GetRandomFileName()` component in the per-user `$env:TEMP` on Windows PowerShell. The random component defeats pre-creation and POSIX `mkdtemp` mandates mode 0700, which gates traversal into the directory regardless of the modes of the files inside it — so the file modes need no separate change. The delete-vs-archive cleanup protocol is untouched, and the two downstream sites that restated the old path formula now refer to the path the primitive returned. ### Portability — fixed here, not deferred An earlier draft of this description shipped `mktemp --tmpdir` and `mktemp -t`, and recorded the gap as a known limitation to file a follow-up for. Review caught it. It is fixed in this PR rather than deferred. **A second review pass then caught the *justification* being wrong, and that correction matters more than the first.** This description — and six sites in the diff — asserted that `--tmpdir` is "absent on BSD". **That is false.** The [FreeBSD `mktemp(1)`](https://man.freebsd.org/cgi/man.cgi?query=mktemp&sektion=1&format=html) synopsis and the macOS/Xcode page both read `mktemp [-d] [-p tmpdir] [-q] [-t prefix] [-u] template ...` — BSD has the directory flag. (The claims below are kept to the short `-p` spelling, which is what both synopses show verbatim; the long `--tmpdir` form is GNU's, and BSD spells it inconsistently across versions.) The "absent on BSD" claim was inherited from an existing comment in this repo and repeated as if verified; it was not. The real hazard is worse than absence, which is why the fix itself stands unchanged. The flag exists in both dialects and **means different things**: - **GNU** treats the positional template as relative to that directory and lets the flag beat `TMPDIR`. Its own manual deprecates `-t` on exactly that ground: "the use of `-p` without `-t` offers better defaults (by favoring the command line over `TMPDIR`)". - **BSD/macOS** consult it only as a fallback **for the `-t` flag** when `TMPDIR` is unset — "If the `-p` option is set, then the given *tmpdir* will be used if the `TMPDIR` environment variable is not set." So `TMPDIR` wins instead, and with a bare positional template and no `-t` the flag does nothing at all. So `mktemp --tmpdir visualize-XXXXXX.html` does not *fail* on macOS. It resolves the template against the **current directory** and silently writes into the consumer's repository — the precise outcome the ephemeral tier's never-in-the-repo rule exists to prevent, and a silent wrong answer rather than a loud one. BSD's `-t` also takes a *prefix* rather than a template, so the two dialects produce different filenames from the same argument. Every call site in this diff therefore carries the temp root in the **positional TEMPLATE** argument, which neither dialect reinterprets: ```bash mktemp "${TMPDIR:-/tmp}/fc-scrape-XXXXXX" mktemp -d "${TMPDIR:-/tmp}/visualize-XXXXXX" ``` The *form* is still the one **this repository already mandates for shell scripts** — `scripts/shell-portability-tokens.txt` lints `mktemp -p` and `mktemp --tmpdir` out of changed `**/*.sh` files and points at the positional TEMPLATE replacement (#1527) — and that gate scanning `.sh` only is exactly how these skill *documents* drifted from it. Note that the gate's own inline comment carries the same "BSD does not implement" error this PR just corrected in its own prose; fixing that comment is #1544's lane, not this PR's, so it is deliberately untouched here. The two skills that hand back an HTML file (`visualization/visualize`, `prototype/explore-directions`) create a private run directory with `mktemp -d` and write a fixed-name page inside it, rather than asking `mktemp` for a template with a `.html` suffix *after* the `XXXXXX`. **That question is now settled — and had it gone the other way this PR would have shipped a macOS-broken instruction.** When these producers were written the suffix form was an open risk (#1709 raised it). #1675's lane has since resolved it against primary sources and written the answer into the tier contract itself: `docs/conventions/topic-docs/README.md` now requires the `XXXXXX` placeholders to be **trailing**, because BSD `mktemp` on macOS substitutes only trailing Xs — so `<prefix>-XXXXXX.html` is not merely non-conforming, the file **cannot be created at all** there. GNU coreutils accepts it and this repo's CI is `ubuntu-24.04` throughout, so no gate here would ever have caught it. Both producers already take the `-d`-plus-fixed-filename form that rule prescribes, so they conform as written. The surrounding prose is upgraded from "we do not depend on the suffix working" to stating plainly that it does not. **On verification, stated plainly.** GNU coreutils 8.32 under Git Bash, executed in this environment: `mktemp -d "${TMPDIR:-/tmp}/es-session-XXXXXX"` → `/tmp/es-session-cmjdnl`, exit 0. (An earlier probe here also showed `mktemp "${TMPDIR:-/tmp}/visualize-XXXXXX.html"` succeeding on GNU — that success is exactly the trap the trailing-X rule now closes, since the same command fails on macOS.) The PowerShell branch was executed too (PowerShell 7.6.4, Windows 11): it creates the directory under `%LOCALAPPDATA%\Temp` and returns its full path. **BSD/macOS is not executable from here**, so every BSD claim above is quoted from the FreeBSD and macOS man pages rather than run — which is the discipline whose absence produced the "absent on BSD" error in the first place. The 0700 mode claim likewise rests on POSIX `mkdtemp`, not on this environment (Git Bash reports 0755 because MSYS emulates POSIX modes over Windows ACLs), and is now scoped to POSIX in the skill text rather than stated flatly across both platforms. What is still **not** safe, and is avoided everywhere in this diff: a **bare** `mktemp` with a relative template resolves against the *current directory*, so `mktemp -u fc-test-XXXXXX` run inside a repo returns `fc-test-tdvDNS` — a file in the consumer's tree, against the tier's own rule. ### Explicitly left alone The audit confirmed these correct, and changing them would be regressions: `quiz-me` and `ai-briefing` (machine state), `audit-pass` (machine-state findings store), `course-digest` and `youtube-digest` (correct Ephemeral/Memory/Machine-state split, verified against the actual `os.tmpdir()` library code), `running-retro` (machine state by design — the observer outlives the session), `machine-health` (durable report archive), `pressure-test` and `prototype/context/discipline.md` (in-repo throwaway *source code*, a deliberate pattern outside this contract), and the `work-items` / `source-control` `mktemp` + `rm -f` spill-file pattern (rule 2's permitted self-consumed exception). ## Test plan Run from the branch worktree against `origin/main`: - `scripts/check-changelog-parity.sh --check-bump origin/main` — **pass** (all five plugins bumped with matching `## [<version>]` entries) - `scripts/check-contract-slice-prune.sh --check-diff origin/main` — **pass**, no path under `docs/topics/` - `scripts/check-skill-portability.sh origin/main` — **pass**, no unexcused coupling tokens - `markdownlint-cli2` over every changed Markdown file — **pass**, 0 errors - `mktemp` forms exercised directly in this environment (GNU results and the BSD caveat quoted above) - `plugins/firecrawl/skills/update/scripts/update.test.sh` — **17/17 pass** after the `update.sh` temp-dir change - `scripts/check-shell-portability.sh origin/main` — **pass** on the changed shell file One executable path changed in this PR: `update.sh`'s run-directory creation, covered by the 17 checks above. Everything else is instruction text in skill bodies and changelog prose, where no test suite applies. CI confirms the full gate set. Review findings from the adversarial passes have been addressed on this PR, not deferred: - the P1 session-directory exposure (predictable name in shared `/tmp`, now created by `mktemp -d`); - the non-portable `mktemp` flag form across every touched call site, plus `skills/update/scripts/update.sh`, whose `-d -t` produced a differently-named run directory on BSD (all 17 of its checks still pass); - the missing path echo in the `context7` example — and the same defect in the new `mktemp -d` snippets, whose randomly-named directory is unrecoverable in the following tool call without it; - the command-specific wording of the `firecrawl` cleanup exception, now genuinely command-agnostic rather than a closed seven-command list that would need editing whenever a command is added; - the false "`--tmpdir` is absent on BSD" premise at six sites, corrected above against the FreeBSD and macOS man pages; - the concurrent-session-safety bullet that still asserted the retired `eventstorming-session-{id}` formula as the anti-collision mechanism; and - the 0700 claim, now scoped to POSIX `mkdtemp` rather than asserted across both platforms. ## Related **Closes #1700** — the ephemeral-producer sweep. This changed while the PR was open: #1700 named two producers (`planning:interview` round tables, `education:teach` lesson HTML), each gated behind an open design question, which is why an earlier revision of this description said the issue was deliberately *not* closed here. #1675's lane has since resolved both — verified in merged `main` rather than taken on report: `planning/skills/interview/context/loop.md` now creates one `mktemp -d "${TMPDIR:-/tmp}/interview-XXXXXX"` run directory per interview, and `education/skills/teach/context/lessons.md` took the outcome #1700 explicitly anticipated ("it was never ephemeral, fix the label") — workspace lesson HTML is now documented as machine state, with only the `primer` page migrated to the ephemeral tier. With those two answered and the five here migrated, the sweep the issue describes is complete. This is otherwise downstream conformance work from the ephemeral tier contract added in #1635 (PR #1675). #1635 is closed by that PR, not this one, so it carries no closing keyword here. **#1709 stays open** and is deliberately not closed by this PR. Its portability question is answered for these five call sites, but its acceptance criteria also require the chosen form to be recorded normatively in `docs/conventions/topic-docs/README.md` and applied to **every** remaining `plugins/**` call site. At least two are outside this diff and still on the old forms: `architecture/improve/actions/deepening.md` (the origin of the `--tmpdir` choice) and `claude-config/skills/audit/scripts/fix-plugin-drift.sh` (`mktemp -t`). That migration and the normative record remain #1709's, and it carries `needs-human` / `status: needs-decision` for a reason. Closes #1700 ## Related - Closes #1700 — the ephemeral-producer sweep. Its two named producers were resolved by #1675's lane; these five are the remainder, so the sweep completes here. - Refs #1675 — the PR that introduced the ephemeral tier row this sweep enforces, and whose lane added the trailing-`XXXXXX` constraint these producers already satisfy. - Refs #1709 — the mktemp portability follow-up; answered for these call sites, left open for the normative record and the marketplace-wide migration. - Refs #1544 — owns the `scripts/shell-portability-tokens.txt` comment that still carries the "BSD does not implement" framing corrected here; deliberately untouched by this PR. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
The topic-docs tier table sorts documents by one question — does anything downstream enforce against this? — which cannot express lifetime. Its finest-grained cell, the memory tier, conflated state that must SURVIVE the session (resume artifacts, ledgers, captures) with files that DIE with it.
With no row naming the second kind, two plugins answered the same unasked question differently:
adhd:clarifyreached for the session scratchpad — an undocumented harness path (zero occurrences in the 6.4 MB docs corpus, keyed by working directory, declined three times upstream as a supported surface: anthropics/claude-code#45745, #17936, #21248, all closed as not-planned) — whilearchitecture:improvehad independently settled on amktemptemp file. That divergence, not a shared mistake, is what the convention registry's trigger anticipates: an owner doc before a second plugin adopts.This adds one additive Ephemeral row plus a section stating its five rules, a re-derivation trigger, and a rationale recording why the other three candidate axes needed no change.
Minor, not major: no tier moves, no
topic-docs.yamlkey renamed, slug spec untouched, and no visibility guarantee changed — the ephemeral row is slug-less and invisible to every other execution context by construction, so it takes no row in the visibility matrix. The eight bindings need no synchronized adoption wave.Scope beyond the original row
Review enumerated further producers that the new row reclassifies. Rather than defer them, this PR places every producer in the plugins it already touched:
planning— the/planning:interviewdense-round tables move out of the memory slice into one OS temp directory per run, and the plugin's four other undocumented HTML views (prdpitch,brainstormreaction-capture,planview,designtopology) get a placement where they previously resolved to nowhere. Both the eagerly-loadedSKILL.mdand the on-demandcontext/loop.mdmove together.education—primer's HTML had no resolvable path at all (it routed through a workspace placement while creating no workspace) and is now ephemeral-tier; the workspace lesson HTML is correctly machine state, and stops calling itself ephemeral while writing into persistent state. An HTML lesson also gained a canonical name (lesson.html, replacinglesson.md, never both) and a<meta name="concept">marker so the slug-collision guard keeps an identity source.The remaining five plugins are swept in #1708.
Acceptance criteria
docs/conventions/topic-docs/README.mdcarries an Ephemeral row with all rules stateddocs/conventions/topic-docs/CHANGELOG.mdrecords the change as MINOR (2.4.0) with the justificationplugins/adhd/skills/clarify/SKILL.mdno longer references the session scratchpadplugins/architecture/skills/improve/actions/deepening.md— the executable step, not only the format reference — resolves its temp path by the new row's rulemktempformBeyond the stated criteria,
plugins/architecture/reference/topic-docs.mdsaid its HTML report was "deliberately ephemeral, outside this convention" — now stale, so it is corrected in the same change rather than left contradicting the new row.Test plan
Run from the branch worktree against
origin/main:npx markdownlint-cli2over all changed markdown — 0 errorsscripts/check-changelog-parity.sh --check-bump origin/main— passscripts/check-contract-slice-prune.sh --check-diff origin/main— pass, no path underdocs/topics/scripts/check-skill-portability.sh origin/main— pass, no unexcused coupling tokensThe
mktempforms were verified against primary sources rather than assumed: FreeBSD and macOSmktemp(1)both document that only trailingXsare substituted, so a…-XXXXXX.htmltemplate cannot create the file on macOS — while GNU coreutils 8.32 accepts it (reproduced locally). This repo's CI isubuntu-24.04throughout, so no gate here would have caught it; every producer now takes the-drun-directory form, which keeps the placeholders trailing.Related
Closes #1635