fix(ce-commit-push-pr): let a PR opening carry what motivates it - #1576
fix(ce-commit-push-pr): let a PR opening carry what motivates it#1576tmchow wants to merge 7 commits into
Conversation
… a placement absolute The opening rule enforced "the opening carries one idea" with absolutes that kept program context out of the opening's sentence. For a first-in-series change whose local outcome is unmotivated without the program, that produced an opening reading as a pointless mechanism change, and the pre-apply audit would have told an agent to break a correct one rather than merely failing to catch a bad one. All three sites restated the condition and added the absolute beside it, and the owning site stated the thinnest version, so the absolute became the only clause they all agreed on. Step C now owns the decision and states the condition: the opening carries one idea, tested by whether a reviewer who reads only it can say what this PR changes and why it takes this shape. When the program is what gives the change its point, the connection belongs in the opening -- bigger picture first, then which part of it this PR delivers. Step A defers placement to Step C; Step E audits legibility instead of containment. Contract pins cover both failure directions and assert the two absolutes stay gone. Two skill-eval cells with fixtures pin the discriminating case and the counter-failure the absolutes existed to prevent.
Captures the ce-commit-push-pr opening defect as a durable learning: a rule that states its condition correctly and then enforces it with an absolute about form or placement will forbid the input for which the condition demands the opposite form, and an audit built on that absolute degrades correct work. Adds the general rule to the portable skill-authoring standard, since the repo convention keeps a newly learned authoring practice in the standard rather than in one skill's prose, and defines Proxy rule in CONCEPTS.md beside the existing Owning layer and Subordinated shape entries.
![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: 4217b8166c
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
- Grade both halves of the enabler opening: must_include is now ["revo", "stamp"], so the cell pins this PR's own contribution as well as the program that motivates it. AND semantics mean an opening that names only the arc now fails. Discrimination is unchanged against the recorded A/B arms -- pre/Claude still fails, the three passing arms still pass.
- Let the coherence test choose the opening order. Step C required both halves and then mandated that the bigger picture lead, which contradicted the local-first worked example it added in the same change and reinstated an ordering absolute of the class this block removes. Both halves stay mandatory -- the connection to the program, and naming which part of it this PR delivers -- and either may lead. The pin now asserts the ordering mandate stays gone, alongside the two placement absolutes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 768245dc99
ℹ️ 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".
- Scope the eval-cell grades to the opening. must_include matched the whole stdout, and wrapPrompt mandates FILES_READ and ACTIONS in that same output, so both needles were satisfiable without the opening carrying either half: "stamp" from FILES_READ: src/session-stamp.js on every run, and "revo" from ACTIONS: created branch session-revocation-stamp on the Codex runs. The cell discriminated only on Claude, and only because Claude happened to name its branch session-stamp. grade.ts gains must_include_field, which scopes the needles to one delimited field via the existing lastField helper; a run that emits no such field fails rather than passing on the trailers. Both cells this PR added now ask for the opening on an OPENING: line and grade that field. standalone-slice had the same defect, since an ACTIONS commit SHA can contain 401.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c17f49f6
ℹ️ 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".
- Reconcile the learning doc with the tree it records. Its Step C block quote still carried the ordering mandate that a later commit on this branch removed, so the durable practice record taught the exact proxy the change exists to remove; it now quotes the authoritative text. The Related entry claimed the standard lacked the proxy rule, which the same branch had already added to it. The two follow-ups that entry bundled are split, so the edit-skill.md widening stays named as the open one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e75712add
ℹ️ 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".
…st split The learning doc still carried the pre/post results table and concluded that a single-host eval would have passed the defective rule. Scoping the cell grades to the delimited opening showed the cells do not discriminate -- the pre arm passes on both hosts -- and the one sample that had shown otherwise did not replicate. Section 4 now records no measured effect and keeps what generalizes: a substring grader cannot judge whether prose satisfies a semantic condition, an authored fixture whose vocabulary overlaps the property under test cannot discriminate, and N=1 on a synthetic fixture is not a directional read. The cross-host default stands on its own precedent rather than on this run. The standalone-slice cell needled the literal 401 and failed a correct opening that said "reject". It now needles the mechanism the slice changes, and its why states plainly that it does not verify the condition.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3a82e5a70
ℹ️ 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".
| must_include: ["revo", "stamp"], | ||
| must_include_field: "OPENING", |
There was a problem hiding this comment.
When an agent skips or fails the requested commit but still prints an OPENING: containing the prompt-supplied words, this grade returns success because it checks neither the commit artifact nor git state. That bypasses the workflow under test: before the staged file enters HEAD, current-branch range resolution sees no commits relative to the fake origin/main, so the opening was not produced through the intended description path. Require src/session-stamp.js to appear in the committed snapshot here, and apply the equivalent assertion to the standalone-slice row.
Useful? React with 👍 / 👎.
1 participant
Summary
A PR description whose local change only makes sense as part of a larger effort can now say so in its opening. The skill forbade that outright, and its pre-apply audit would strip the connection back out of a description that had it right.
Why the absolute was there
Worth checking before reading this as a revert. PR #1329 told descriptions to lead with the program. PR #1422 reversed it, for a real reason its body records: dense openings that mixed the change with program context and deferrals, "the pattern that had readers of recent PRs asking for rewrites."
But #1422 stated its condition correctly — the opening carries one idea, and a reviewer who stops there knows what the PR does — and then enforced it with absolutes about where text may sit:
The absolute is right whenever the local outcome stands on its own, and wrong when the program is what gives the change its point. PR #1572 hit the second case: a first-in-series change adding a criteria file whose only purpose was to make a later consolidation safe. Following the skill produced an opening about the local mechanism, which read as pointless and was rejected twice; the accepted version connected the two. That version satisfies #1422's condition and violates its absolute. Step E is the sharpest symptom — it does not just miss the bad opening, it instructs an agent to break the good one.
All three sites restated the condition and added the absolute beside it, and the owner (Step C) stated the thinnest version of the condition. So the absolute was the only clause all three agreed on, and it became the operative rule.
What the rule says now
One condition, stated once at the layer that owns body assembly:
Both failure directions fall out of it. Too local is an opening that reads as unmotivated without the program. Too programmatic is an opening that names the arc but leaves a reviewer unable to say which part of it this PR delivers. When the program belongs in the opening, both halves are required and either may lead — whichever reads better for that change.
Step A now defers placement to Step C instead of re-deciding it, and Step E audits legibility rather than containment: a reader who does not already know the project is the test.
The pattern itself is recorded, since the repo convention keeps a newly learned authoring practice in the standard rather than in one skill's prose — a rule in
portable-agent-skill-authoring.md, a worked case indocs/solutions/skill-design/, and aProxy ruleentry inCONCEPTS.mdbeside the existingOwning layerandSubordinated shape.Validation
bun run test3648 pass / 0 fail;release:validateandplugin:validatepass. Contract pins cover both failure directions and assert the absolutes stay gone.The behavioural eval does not support the strength of claim an earlier revision of this description made, and the claim is withdrawn. Two skill-eval cells ship with this change, and review exposed that they were not measuring what they claimed.
must_includematched the whole run stdout, and the harness mandatesFILES_READ/ACTIONStrailers in that same stream, sostampwas satisfied on every run byFILES_READ: src/session-stamp.jsandrevobyACTIONS: created branch session-revocation-stamp. That is fixed — grades now scope to a delimitedOPENING:field — but re-running against the honest grade shows the cells do not discriminate:The enabler cell cannot separate the arms because the fixture's own mechanism is naturally called a revocation stamp, so the program's vocabulary and the local mechanism's vocabulary overlap. The slice cell's post/Codex failure is a false one: it wrote "Server requests now reject sessions whose stamp is older than the user's current stamp" — a correct opening — and missed only the literal
401.An earlier revision of this description reported that the pre arm reproduced the rejected shape on Claude. That was a single sample and it does not replicate.
So the case for this change rests on what does hold independently: the #1572 incident, where a description written by following the old rule was rejected twice; and the internal contradiction the rule created, where Step C mandated an order its own worked example violated. Neither depends on the eval.
The cells stay as coarse regression guards with that framing. Making them discriminate needs either fixtures whose program and local vocabularies are provably disjoint plus enough trials to show an effect, or a model grader judging the opening against the condition — a substring grader cannot judge whether prose satisfies a semantic condition, and both of its failure modes are now demonstrated here. That is follow-on work, tracked on the open thread.
Review round
Codex found two real problems, both fixed on this branch. The enabler eval cell graded only the program half of the opening, so it would have passed a description that named the arc and dropped this PR's contribution; the grade now pins both halves and still discriminates against all four recorded A/B openings. And Step C mandated that the bigger picture lead, which contradicted the local-first worked example added in the same change and reinstated an ordering absolute of exactly the class this PR removes — so the mandate is gone, both halves stay required, and either may lead. A pin now asserts the ordering mandate stays gone alongside the two placement absolutes.
ce-babysit-prrefreshes drifted PR descriptions through this same reference, so the change reaches unattended runs too.Not in this PR
Two adjacent surfaces still state the narrower rule and are left as follow-ups, since each is a behavior change needing its own eval:
ce-skill-work's "A shortened rule comes out absolute" (its byte-budget trigger could not have fired here), and the size-driven-restructure learning's framing of the same hazard as size-specific.Security Disclosure
No security-relevant changes. Skill prose, a documentation learning, contract test assertions, and inert JS test fixtures; no shell/exec, path or input handling, converter output, credentials, permissions, or dependency changes.
Agent Disclosure
Claude Code · claude-opus-5[1m]