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 6 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".
| // opening can only carry from the program context, never from the diff. | ||
| // "stamp" is this PR's own contribution — the staged module's mechanism — which | ||
| // an opening that names only the arc has no reason to mention. | ||
| must_include: ["revo", "stamp"], |
There was a problem hiding this comment.
Fresh evidence after the earlier grade change is that must_include searches all of stdout, while wrapPrompt mandates FILES_READ and ACTIONS trailers in that same output. Consequently, an opening that mentions only revocation can still satisfy "stamp" via a trailer such as FILES_READ: src/session-stamp.js, so requiring both needles still does not establish that both halves occur in the opening. Extract or delimit the opening and grade that field alone so this fresh-agent eval can actually detect the regression it claims to cover.
AGENTS.md reference: AGENTS.md:L162-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Extract or delimit the opening and grade that field alone so this fresh-agent eval can actually detect the regression it claims to cover.
Scoping is done in 26c17f4, and it was worse than you found — both needles leaked, by two paths. stamp came from FILES_READ: src/session-stamp.js on every run, as you said. revo also leaked, on both Codex runs, from ACTIONS: created branch session-revocation-stamp. The cell only discriminated on Claude, and only because Claude happened to name its branch session-stamp.
grade.ts now takes 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 ask for an OPENING: line and grade that field.
But the second half of your ask — "so this eval can actually detect the regression it claims to cover" — is not met, and scoping is what made that visible. I re-ran both cells A/B on Claude and Codex against the honest grade:
- Enabler cell: no discrimination. The pre arm now passes on both hosts. pre/Claude: "The accounts service now keeps a monotonic revocation stamp per user, with read and bump operations. Nothing reads it yet." pre/Codex: "This first slice establishes a monotonic per-user session stamp, giving the server-side revocation project shared state..." The
revoneedle cannot separate the arms, because the fixture's own mechanism is naturally called a revocation stamp — the program's vocabulary and the local mechanism's vocabulary overlap. - Slice cell: a false fail. post/Codex wrote "Server requests now reject sessions whose stamp is older than the user's current stamp, turning PR 1's per-user stamp into enforced server-side revocation" — a correct opening — and failed only because the needle is
401and it wrote "reject".
The honest reading is that my earlier report of pre/Claude reproducing the rejected shape was one sample and does not replicate. So this PR's validation section overstates its evidence, and I'm correcting it rather than leaving the claim standing.
The root is approach-level, not another needle: a substring grader can't judge whether a prose opening satisfies a semantic condition. Both failure modes are now demonstrated — vacuous pass (the needle is satisfied by vocabulary the fixture supplies anyway) and false fail (the needle is one of several valid wordings). Tuning a third needle is the whack-a-mole, so I'm leaving this open for a call on which way to go rather than fixing past you.
- 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".
| | `enabler-opening-carries-the-program` | pre | Claude | FAILED — "A per-user session stamp now exists as a single readable, incrementable value. Nothing consumes it yet ... so this PR changes no behavior on its own." The exact #1572 rejected shape. | | ||
| | `enabler-opening-carries-the-program` | pre | Codex | PASSED — "This adds the monotonic per-user stamp that server-side session revocation will build on. It is the first of three PRs..." Codex ignored the absolute. | | ||
| | `enabler-opening-carries-the-program` | post | Claude | PASSED | | ||
| | `enabler-opening-carries-the-program` | post | Codex | PASSED | | ||
| | `standalone-slice-keeps-its-outcome` | pre and post | both | PASSED — no run led with the arc and lost the local outcome. | |
There was a problem hiding this comment.
The commit's Validation section says the honest rerun did not reproduce the enabler pre-arm failure and that the standalone slice failed on post/Codex, but this durable solution record still claims the exact opposite: a Claude pre-arm failure, all post arms passing, and both slice arms passing. That leaves the documented cross-host evidence and the lesson derived from it factually stale; update this table and the conclusions below it to match the final rerun rather than the earlier invalid grade.
AGENTS.md reference: AGENTS.md:L164-L166
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]