fix(ce-pr-description): hand off PR body via temp file#581
Merged
Conversation
… temp file
Change ce-pr-description's output contract from {title, body} to
{title, body_file}. The body is written to an OS temp file via
mktemp + heredoc and the caller substitutes it back with
"$(cat "$body_file")". This avoids emitting the body text twice
(once as the skill's output, again when the caller formats the
gh pr command), which was wasteful on large PR descriptions.
Also add a narrow AGENTS.md exception for value-producing
preparatory commands (VAR=$(cmd1) && cmd2 "$VAR"), so the skill
can use the natural mktemp + heredoc pattern without manually
threading the path across two bash calls.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a Rationale Discipline subsection to the plugin AGENTS.md skill compliance checklist. The rule: every line in SKILL.md loads on every invocation, so rationale earns its space only when it changes what the agent does at runtime. Keep rationale at the highest-level location that covers it; restate behavioral directives at the point they take effect so long skills don't drift by line 400. Portability notes, defenses against mistakes the agent wouldn't make, and meta-commentary about repo rules go in commit messages or docs/solutions/, not the skill body. Apply the rule to ce-pr-description and git-commit-push-pr: Step 9 shed three bullets of archaeology (why mktemp -u, sentinel choice, why not Write), the AGENTS.md cross-reference, and the double-tokenize rationale repeated across four locations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![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: 1ba5ef3b86
ℹ️ 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".
Address PR review feedback (#581) - `mktemp -u -t ce-pr-body` fails on GNU mktemp (Linux/Codex) with "too few X's in template". Switch to bare-template form `mktemp -u "${TMPDIR:-/tmp}/ce-pr-body.XXXXXX"`, which works cleanly on both BSD (macOS) and GNU (Linux) mktemp.
tmchow
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ee9b1312e
ℹ️ 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".
…rite Address PR review feedback (#581) - Drop `mktemp -u`: the -u flag only prints a candidate path without creating the file, which is a TOCTOU race — another process can create or symlink that path before `cat > "$BODY_FILE"` runs. Using `mktemp` without -u creates the file atomically with mode 0600, so the heredoc writes into a path this process already owns. - Chain `echo "$BODY_FILE"` behind the heredoc write with `&&`. The previous layout ran echo on the next line regardless of whether `mktemp` or the heredoc succeeded, so a failure would surface a path to a missing/partial file and break the caller later in `gh pr create/edit`. Chaining propagates the failure cleanly.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Generating a PR description used to tokenize the body twice: once as the skill's returned text, then again when the caller heredoc'd it into
gh pr create/edit. On large PRs that compounds. Now the skill writes the body to an OS temp file and the caller reads it back with$(cat "$BODY_FILE"). Body tokenized once, caller's tool input shrinks to just the path.Reviewing that commit surfaced a second problem: the new Step 9 grew three bullets of design archaeology (why
mktemp -u, why notEOF, why not theWritetool). Every line ofSKILL.mdloads on every invocation, so that archaeology carries forward forever. A newAGENTS.mdrule, Rationale Discipline, formalizes the principle that drove the cuts so future skill authoring has the guardrail.What changed
ce-pr-descriptioncontract moved from{title, body}to{title, body_file}. Step 9 writes the composed body viamktemp -u+ heredoc and prints the path.mktemp -usidesteps Claude Code'sWritesandbox (which can't touch/tmpand refuses to overwrite unread files). The quoted heredoc sentinel keeps$VAR, backticks, and literalEOFin the body from clashing with the terminator. OS reaps/tmpon its own, so no cleanup.git-commit-push-prcallers read the body from the returned path. DU-3 (description-only) and Step 7 (full flow, both new and existing PR) substitute with$(cat "$BODY_FILE")insidegh pr create/edit. DU-3's compare-and-confirm note explicitly says not tocatthe file when summarizing differences: the body is already in context from the bash call that wrote it.New
AGENTS.mdauthoring exception for value-producing preparatory commands (VAR=$(cmd1) && cmd2 "$VAR"). The "no action chaining" rule used to block this pattern. The new exception permits it whencmd2strictly consumescmd1's output and splitting would force manually threading the value through model context across two bash calls.New
AGENTS.mdRationale Discipline rule. Include rationale only when it changes what the agent does at runtime: if behavior wouldn't differ without the sentence, cut it. Keep rationale at the highest-level location that covers it; restate behavioral directives at the point they take effect (long skills drift if constraints sit at line 9 while the agent executes line 400). Portability notes, defenses against mistakes the agent wasn't going to make, and meta-commentary about repo authoring rules belong in commit messages ordocs/solutions/, not in the skill body.Concrete cuts applying the rule. Step 9's three bullets of design notes lost two (kept one load-bearing line on sentinel semantics). A cross-reference to the new AGENTS.md exception got cut as pure meta-commentary. The "avoid double-tokenize" rationale, repeated across 4+ locations in the two skills, collapsed to one mention in the
ce-pr-descriptionfrontmatter.Why these ship together
The Rationale Discipline rule was written in direct response to reviewing the first commit's Step 9; the cuts it demands ARE the first application. Splitting would mean either committing a rule with no evidence it matters or committing cuts with no explanation of the principle. Together they carry the full compound arc: refactor, then lesson caught on review.