feat(instruction-placement): persist the delta baseline in the artifact-protocol baselines slot - #3831
feat(instruction-placement): persist the delta baseline in the artifact-protocol baselines slot#3831kyle-sexton wants to merge 2 commits into
Conversation
…ct-protocol baselines slot The delta lane kept its baseline behind `lib/state-key.sh`, whose key ends in a hash of the worktree's absolute root path. The sibling overengineering delta persists the equivalent state through the shared lifecycle artifact protocol, so the marketplace carried two ways of keeping one kind of state and the state-keyed one lost every operator decision at a checkout boundary. instruction-placement becomes a protocol participant. It ships `reference/artifact-protocol.md`, registered in `validate-plugin-contracts.mjs` so the copy stays byte-identical to the canonical doc, and a new `reference/topic-docs.md` binding that owns placement. Both artifacts move to a memory-tier, branch-keyed home: `findings.md`, and `baselines/delta-baseline.md` in the protocol's named `baselines/` slot. `instruction-placement` joins the convention's reserved concern names (topic-docs 3.2.0, additive). delta now compares against that persisted baseline rather than against the artifact, which every audit merges into and every realign edits, and the baseline carries the declined records forward as records of their own. A tree at the retired location is not migrated and not read: a read-side fallback would keep the worktree-hashed derivation alive as the parallel second home this change closes. The absence is made detectable instead, by naming the resolved home that came up empty and routing to a full audit. `scripts/artifact-home.test.sh` pins the read path and the write path to one slot across every shipped surface. A capture step and a read step that name different files raise no error at runtime, so that disagreement is caught here. Closes #3811 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
…slot per step As shipped, nothing ever wrote the first baseline. `audit` and `realign` write none, the end-of-cycle capture is earned only by a cycle that consumed one, and a missing baseline routed out. Every delta run after an audit was therefore a permanent route-out and no cycle could compare. `delta` step 1 now disposes of what it finds the way the sibling `overengineering:delta` does: baseline present and branch-matched is the baseline; `findings.md` present with no baseline (absent, or a `branch:` mismatch) is a bootstrap cycle that captures the artifact's spine rows and declined records over `baselines/delta-baseline.md` before it compares, then compares against that capture and says so; only a missing artifact routes out. The one departure from the sibling is forced by composition: overengineering's delta runs the audit itself, so its no-artifact case runs the audit and captures afterwards, while this delta does not compose `audit` and keeps routing to it. The route-out message now names the retired pre-0.12.0 plugin-data tree as the likely cause, in one clause and without reading it, so a home the upgrade left behind and a plugin that never ran no longer read the same. The contract, the binding, and the 0.12.0 changelog entry state the bootstrap and the narrowed route-out, and the binding's three bare `delta-baseline.md` mentions now carry the `baselines/` slot. `scripts/artifact-home.test.sh` was vacuous in the read direction: the sweep whitelisted the bare leaf, and the both-steps assertion counted mentions across the whole file (>= 2 of 3), so the hard rule covered for a mutated step. The bare-leaf allowance is gone, and the read step and the capture step are each located under `## Workflow` and checked on their own: every baseline path inside the step must end in the slot. Mutating the read step alone to `delta-baseline.md` now fails two assertions; mutating the capture step alone fails two; restored, the suite passes with 21 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
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. |
Independent review and the fixes it forced — flipped to readyPosted by the autonomous worker lane ( The blocking finding: the feature was a dead end as first written
Fixed by adding a bootstrap cycle in One forced departure, stated in the commit, the contract and the body rather than smoothed over: The second finding: the guard test only looked like it guarded
Now: bare-leaf allowance dropped, the whole-file count replaced by per-step checks that locate the read and capture steps under
Confirmed sound by the review, and left aloneWrite and read paths agree on Still partial, and said soIssue criterion 2 (a declined record surviving a different worktree) stays unmet under the default per-checkout memory root. The worktree-path hash is gone and declined records ride in one named file, but full cross-checkout durability needs a tracked suppression surface. The absent-baseline message now also names the retired pre-0.12.0 plugin-data tree as a likely cause, without reading it, so an upgrade that lost a baseline is distinguishable from a plugin that never ran. Gates: Generated by Claude Code |
|
Claude finished @kyle-sexton's task in 2m 33s —— View job Security review complete
No security issues found. What this PR touchesThis is a state-persistence refactor for the Areas checked
No injection, path-traversal, authz, or secret-handling concerns identified in the diff. |
|
Claude finished @kyle-sexton's task in 4m 41s —— View job Code review —
|
|
Last security-reviewed head: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - **Both present, `branch:` does not match.** No baseline, naming both branches. The stored file | ||
| is kept, not overwritten, and the run takes the bootstrap path below. | ||
| - **`findings.md` present, no baseline.** A **bootstrap cycle**. Capture the artifact's spine | ||
| rows and its declined records, per the contract's capture rules, over | ||
| `baselines/delta-baseline.md` in the same home **before comparing anything**, then continue | ||
| with that capture as this cycle's baseline. Say in the report that this is a bootstrap and | ||
| that a status change made before it is unobservable this cycle. This is the only path by | ||
| which a first baseline comes to exist: `audit` and `realign` write none, so the delta run | ||
| after any audit takes it. |
There was a problem hiding this comment.
Branch-mismatch bootstrap silently destroys the other branch's baseline, contradicting both this step and the contract it's owned by.
Step 1 disposes of a branch: mismatch by saying the stored file "is kept, not overwritten, and the run takes the bootstrap path below" (lines 87-88). But the bootstrap path it points to (lines 89-95) captures "over baselines/delta-baseline.md in the same home before comparing anything" — the identical path. Since this plugin enforces exactly one stable baseline path per home (no per-branch filename, and artifact-home.test.sh pins that there is only ever one canonical path across the whole surface), there is nowhere else for that capture to land. The bootstrap write therefore overwrites the very file this bullet just said would be kept.
This also contradicts the contract this skill defers to, context/findings-artifact.md lines 173-179: "a branch: mismatch keeps the mismatched file too, since the bootstrap that follows is a separate write the mismatch does not earn a replacement of" — but no separate location is ever defined anywhere in the binding or the contract; the single-slot design (by construction) makes "separate write" unimplementable.
Concretely: two branch names that slug to the same directory (an explicitly acknowledged, documented case — "The slug mapping is lossy and two branch names can slug to one directory") will have branch A's delta run capture a baseline, then branch B's next delta run silently clobber it via the bootstrap-before-compare write, destroying branch A's declined records — the exact loss this whole baseline redesign exists to prevent ("A decline is the one judgment in this plugin that no other surface can reconstruct"). This also violates the skill's own hard rule at line 156: "A baseline the run did not consume is never overwritten."
artifact-home.test.sh only checks static path strings across the shipped docs, so it wouldn't catch this runtime behavior gap.
![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: 3b0f6e4db6
ℹ️ 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".
| **Persisting at rungs 2 to 4 is ask-gated, never automatic.** Each of those rungs persists the | ||
| resolution to the concern file only on the user's explicit confirmation; declining is a valid answer | ||
| that leaves the resolution session-local, and the run proceeds either way. This is the one sanctioned | ||
| tracked write of the audit skill, whose read-only headline is scoped to unasked writes; every unasked | ||
| write it makes stays in the memory tier. |
There was a problem hiding this comment.
At resolution rungs 2–4, this now instructs audit to persist .claude/topic-docs.yaml, but skills/audit/SKILL.md:166-167 still says its findings artifact is the only file it writes and forbids repository edits. The claimed scoping of the read-only headline was therefore not carried into the skill body, leaving first-run audits with mutually exclusive instructions: either skip the required persistence or violate their hard rule. Update the audit contract to disclose the explicitly confirmed concern-file write, or remove that write from this resolution flow.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| - **`findings.md` absent.** Route out. Name the resolved path you looked in, say that no | ||
| artifact is there, and route to the full audit rather than silently running one. On a | ||
| freshly upgraded plugin, name the retired pre-0.12.0 plugin-data tree as the likely cause, | ||
| in one clause and without reading it: a home the upgrade left behind and a plugin that never | ||
| ran look identical from here, and that sentence is what lets an operator tell them apart. |
There was a problem hiding this comment.
On every missing-artifact run, this tells the model to diagnose and narrate a specific pre-0.12.0 migration. That is version-specific historical guidance in an always-loaded skill body, contrary to the repository convention that skill bodies carry only the current rule and reason. Keep the current behavior—name the missing resolved home and route to an audit—but leave the retired path and version history in the reference document and changelog.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| - **Both present, `branch:` does not match.** No baseline, naming both branches. The stored file | ||
| is kept, not overwritten, and the run takes the bootstrap path below. | ||
| - **`findings.md` present, no baseline.** A **bootstrap cycle**. Capture the artifact's spine | ||
| rows and its declined records, per the contract's capture rules, over | ||
| `baselines/delta-baseline.md` in the same home **before comparing anything**, then continue |
There was a problem hiding this comment.
When two branch names collide under the documented lossy slug mapping, this path first requires the mismatched baseline to be kept, then directs the bootstrap to write over that same canonical file. Overwriting destroys the other branch's unconsumed baseline; honoring the keep rule means the current branch never establishes a usable baseline and bootstraps again on every run. The mismatch case needs to route out or resolve a collision-disambiguated home rather than taking this overwrite-based bootstrap path.
Useful? React with 👍 / 👎.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Merge lane claim — canonical for #3811, and why I am not advancing it to mergeClaimed at head Duplicate resolved#3833 worked #3811 independently from a second worker-lane session, overlapping this PR on thirteen files including the identical
|
|
Superseded by #3889, with evidence rather than a coin flip between duplicate lanes. This PR is currently conflicting against main, and a diff check shows it carries neither of two fixes that came out of review on the other lane:
#3889 also carries three further review fixes from the same rounds: It is rebased onto current main, with the config-cascade implementers conflict resolved by keeping both the Closing this one is the cheaper reconciliation. If any of the above is wrong, say so on #3889 and I will fold this branch's version back in instead. |
…d set across the right tiers (#3889) ## What Moves the instruction-placement delta skill's persistence off the ephemeral state key: the diff spine goes to the artifact-protocol `baselines/` slot, branch-keyed, and declined findings go to a tracked finding-suppression surface that git carries between checkouts. Closes #3811. > Supersedes #3833, which the babysit loop closed as a duplicate of #3831. That was reasonable at the time, but #3831 is conflicting against main and carries neither of the two review fixes this branch has, described below. A closed PR does not track later pushes, so the work sat stranded on the branch. This PR is that branch, rebased onto current main. ## Why the first attempt was wrong The original issue asked for the baseline to move to the artifact-protocol slot "the same way the overengineering delta skill uses", expecting a declined finding to survive into another worktree from there. The sibling plugin does not work that way, and the destination cannot satisfy the requirement. The topic-docs convention states that a memory document is visible "only in the checkout that wrote it", marks a sibling worktree `invisible`, and refuses to carry that file class with `.worktreeinclude` ("never baselines or raw scratch"). Git is the only mechanism that crosses checkouts. So the two concerns split: the diff spine stays in the memory tier where a per-branch artifact belongs, and the operator's judgment rides a tracked file under the marketplace's finding-suppression contract, which is exactly what the sibling plugin does. ## What review caught, across three rounds - **The declined set never reached the baseline at all.** `realign` wrote `declined` only into the branch-scoped findings artifact, and `delta` mirrored from that same artifact, so the criterion's own sequence never propagated a decline. Fixed by making `realign` write to the suppression surface, where the decline originates. - **The spine was not actually branch-local.** The branch axis lived in prose while resolution rungs 1 and 5 stopped at the slice root, so two branches sharing a resolved memory home would compare snapshots describing different trees. Every rung now carries the branch segment, and no branch identity resolves no home rather than falling back. - **A discovered finding was reported once and then silently absorbed.** A `new` or re-derived `changed` finding advanced the spine without being persisted anywhere `realign` could act on it, so the next run no longer saw it as movement. A merge step now runs before the spine capture, under a hard rule that the baseline never advances past an unrecorded discovery. - **Glob-validation state was not carried.** With a rule file and its glob text both unchanged, a previously-broken glob's state was lost, so a re-run could not tell a still-broken glob from a newly-broken one. A `RULE` row now carries the glob's validation verdict as of the capture, and `broken-glob` is a transition rather than a state. - **The bootstrap case was unhandled.** Step 1 enumerated only two baseline/artifact combinations and omitted the ordinary one after a first `audit` then `realign`, which resurrected a declined finding. All four combinations are now tabled, and only the both-absent row routes out. - Plus a schema increment the document's own stability rule required, a recorded anchor-collision trade, and an em-dash purge on surfaces that declare it. ## Verification `Plugin contracts validated: 3332 plugin files checked` · `CHECK-SKILL delta|realign|audit|check|setup: PASS — 0 errors, 0 warning(s)` · `check-evals-quality: PASS` · `Linting: 22 files / Summary: 0 issues in 0 files` · changelog parity and newest-first both exit 0 · `No unregistered or drifted cross-plugin source clusters found.` · `All 4 plugin copies match plugins/claude-config/lib/state-key.sh.` ## Rebase notes Rebased onto current main through conflicts in the plugin manifest, the plugin changelog, and the config-cascade implementers table. The implementers conflict was resolved by keeping both rows: `authoring-formats`, which merged today in #3874, and `instruction-placement`, which this branch adds. They are different surfaces. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_012jiwedVq2GxuzN7siXQbr4 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Closes #3811
Summary
The
instruction-placement:deltalane kept its baseline behindlib/state-key.sh, whose key ends in a hash of the worktree's absolute root path. The siblingoverengineering:deltapersists the equivalent state through the shared lifecycle artifact protocol, so the marketplace carried two ways of keeping one kind of state, and the state-keyed one lost every operator decision at a checkout boundary.This makes
instruction-placementa protocol participant and moves both of its artifacts onto the protocol's memory tier. What the delta skill detects, its noise budget, and its report shape are unchanged, per the issue's out-of-scope list.Fix
The implementation follows
plugins/overengineeringfor the branch-keyed, concern-scoped memory home resolved through a plugin-localreference/topic-docs.mdbinding, withreference/artifact-protocol.mdshipped as a byte-identical copy ofdocs/PLUGIN-ARTIFACT-PROTOCOL.md. It is not a byte-for-byte parallel: overengineering keeps a flatspine-baseline.mdin its home root, while this plugin puts its baseline in the protocol's namedbaselines/slot (the slotverificationalready uses), which is what the issue title asks for and what keeps the two names the protocol defines doing the job they were defined for. The home shape and the branch axis are shared; the leaf placement is this PR's own choice.plugins/instruction-placement/reference/artifact-protocol.md, registered inscripts/validate-plugin-contracts.mjs(lifecycleProtocolCopies) so drift fails CI.plugins/instruction-placement/reference/topic-docs.md: the placement binding, deltas only, owning the rung order, the branch slug, the runtime guards, and the retirement note..work/instruction-placement/<branch-slug>/:findings.md, andbaselines/delta-baseline.md.deltanow compares against that persisted baseline instead of against the findings artifact. The artifact is merged into by every audit and edited by every realign, so diffing it against itself measured whatever last touched the file. The baseline is captured at the end of a cycle, only by a cycle that consumed one, and carries the declined records forward as records of their own.auditandrealignwrite no baseline and the end-of-cycle capture is earned only by consuming one, so as first shipped nothing ever wrote the first baseline and every delta run after an audit was a permanent route-out.deltastep 1 now disposes of what it finds the wayoverengineering:deltastep 2 does: baseline present and branch-matched is the baseline;findings.mdpresent with no baseline (absent, or abranch:mismatch) is a bootstrap cycle that captures the artifact's spine rows and declined records overbaselines/delta-baseline.mdbefore it compares, then compares against that capture and says so in the report; only a missing artifact routes out. The one departure from the sibling is forced by composition: overengineering's delta invokes the audit itself, so its no-artifact case runs the audit and captures afterwards, while this delta does not composeauditand keeps routing to it. The contract (context/findings-artifact.md), the binding, and the changelog state the same disposition.lib/state-key.shis removed from this plugin, its threeallowed-toolsgrants are gone, andscripts/sync-state-key.shdrops it from the cluster (four copies remain).instruction-placementjoins the topic-docs convention's reserved concern names; the convention goes to 3.2.0, additive, and gains an Implementers row. The reserved-name rule now distinguishes "flat" (no slice tree) from "no subdirectory ever", sincebaselines/is the first concern-scoped use of the protocol slot.Migration: nothing is migrated, and the absence is made detectable. Reading the old plugin-data tree would require keeping the worktree-hashed derivation alive as the parallel second home this change exists to close, which the issue explicitly forbids ("removed, not left in place as a fallback"). Instead the first
deltaorrealignrun after upgrading names the resolved home it looked in and routes to a full audit, rather than silently reporting a first run. The route-out message now also names the retired pre-0.12.0 tree as the likely cause, in one clause and without reading it, so a home the upgrade left behind and a plugin that never ran no longer look identical. After that audit, the nextdeltabootstraps its baseline from the artifact. The old tree is inert and can be deleted; the binding, the CHANGELOG, and the plugin README all say so.One thing to look at, stated rather than glossed. The epic's cross-worktree criterion is delivered as far as the mechanism honestly goes and no further. The worktree-path hash is gone, so the home varies with the branch and the configured memory root rather than with where a checkout sits, and two checkouts resolving the same
memory_diron the same branch now share one home, which the state key made impossible by construction. Under the documented default the memory root is inside the checkout, so two checkouts still hold two homes; what changed is that a declined decision lives in one named file rather than behind an unreproducible hash. Full cross-checkout durability for an operator judgment needs a tracked suppression surface (docs/conventions/finding-suppression/, the shapeoverengineeringuses), which this PR does not add and which is recorded as a revisit trigger in the plugin README instead of being implied.Verification
New suite
plugins/instruction-placement/scripts/artifact-home.test.sh(21 assertions) grades the documents, following theplugins/discovery/scripts/contract.test.shprecedent. Its load-bearing assertions catch this change's own failure mode: every baseline path named anywhere in the shipped surface must end inbaselines/delta-baseline.md(the bare leafdelta-baseline.mdis no longer tolerated, since a bare leaf at the read step means "besidefindings.md", the second slot the binding warns about), anddelta's read step and capture step are each located under## Workflowand checked on their own, every baseline path inside the step carrying the slot. A write path and a read path that disagree raise no runtime error at all: the lane would bootstrap from the artifact on every cycle while depositing a capture nobody reads, indistinguishable from a healthy first delta after an audit.Mutation-checked in both directions on the tightened suite, each mutation alone and restored byte-for-byte afterwards: rewriting only the read step's slot to
delta-baseline.mdfailsevery baseline path in the shipped surface resolves to baselines/delta-baseline.mdanddelta's read step names baselines/delta-baseline.md(2 assertions, exit 1); rewriting only the capture step's slot fails the sweep anddelta's capture step names baselines/delta-baseline.md(2 assertions, exit 1). The earlier suite passed the read-step mutation, which is the vacuity the review found.scripts/affected-tests.sh --runplugins/instruction-placement/**/*.test.shdirectlyplugins/instruction-placement/scripts/artifact-home.test.shscripts/check-purged-em-dashes.shnpx markdownlint-cli2 "plugins/instruction-placement/**/*.md"scripts/check-changelog-parity.sh --check/--check-order/--check-bump origin/main/--check-preserved origin/maincheck-skill.sh plugins/instruction-placement/skills/deltascripts/check-cross-plugin-source-drift.sh --checkscripts/sync-state-key.sh --checknode scripts/validate-plugin-contracts.mjsshellcheck+shfmt -don the suitedelta's description changed in the first pass (it now writes two files, not one), so the listing budget was re-measured then: aggregate 140855 → 140880 chars, +25, and the per-entry cap is 806/1536. The second pass leaves the description byte-unchanged, so no re-measure is owed. The fleet-wide WARN against the 8000-char documented default is pre-existing and unmoved by this PR.Pre-existing and not from this change:
plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh(process-budget assertions) fails identically on a stashed, pristine tree, which is whyscripts/check-changed-skills.sh origin/mainreports one failing skill.Neither
docs/SKILL-CHEAT-SHEET.mdnordocs/CATALOG.mdneeded a row: both are generated, and nosummaryor plugin description changed. No conflict with the concurrent #3810 lane.Related
baselines/slot as this PR's own departure from its flatspine-baseline.md:plugins/overengineering/reference/topic-docs.md,plugins/overengineering/skills/delta/context/baseline-model.md, and the bootstrap disposition inplugins/overengineering/skills/delta/SKILL.mdstep 2.docs/conventions/topic-docs/3.1.0 → 3.2.0, additive.🤖 Generated with Claude Code
https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob