Fix two stale doc claims from the positional-pop move, and stop treating semver as a review concern - #786
Conversation
`SessionService.popRunbook` was removed from the public surface of
`@rundown-org/core`, which the changeset's own text already called out as
"a breaking change only for a consumer calling `SessionService.popRunbook`
directly". A `patch` bump does not describe that.
The repo's precedent for this exact shape — removing public core API whose
in-repo callers were already retired — is `major`:
`.changeset/delete-completion-and-delegation-locks.md` ("the modules were
dead code with a live public export ... the public API does change, and
that is the breaking part of this major") and
`.changeset/single-store-public-api-cutover.md`.
No release impact either way: five core majors are already queued, so the
next release resolves to 2.0.0 regardless. This is accuracy of the release
record, not a fix to a reachable break.
Both are doc-only; no behaviour changes. `releaseFromSession` justified its in-place shape with "composite operations — releaseRunbooks, popRunbookIfActive — can release several runbooks against one session snapshot". `popRunbookIfActive` releases exactly one run, so the substituted name did not support the sentence's own argument. Attribute the multi-release case to `releaseRunbooks`, and state separately why the conditional pop wants the same primitive: its release folds into the transaction that decides whether the run is still the top. `popTopOfStackUnverified`'s `@returns` promised null "when the stack was empty or the release removed nothing". The second arm is unreachable, as the comment inside the body already argues — `topId` is read off `defaultStack`, so `projectRunbookRelease` always removes it. Dropping that arm alone would leave a second inaccuracy, because null is also the answer when the popped run was the last entry (the wf3/wf2/wf1 unwind in session-service.test.ts covers exactly that), so name both real cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (16)
🧰 Additional context used📓 Path-based instructions (5)**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,json,css}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/core/src/runbook/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
packages/core/src/runbook/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (7)📚 Learning: 2026-02-26T06:27:17.519ZApplied to files:
📚 Learning: 2026-03-13T07:37:13.038ZApplied to files:
📚 Learning: 2026-04-06T04:20:01.297ZApplied to files:
📚 Learning: 2026-05-03T01:45:03.732ZApplied to files:
📚 Learning: 2026-05-15T01:37:58.087ZApplied to files:
📚 Learning: 2026-07-25T01:57:23.567ZApplied to files:
📚 Learning: 2026-04-24T01:48:14.441ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThis PR clarifies session release and stack-pop documentation. It also changes the ChangesSession contract updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR only corrects release metadata and documentation without changing runtime behavior. The listed checks and full test suite pass, and no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🧬 Mutation score (advisory)Hybrid mutation analysis: source changes use changed-line scopes (dedicated tests by default; ℹ️ Mutation scope planSource test selection: ✅
|
| File | Score | Status |
|---|---|---|
src/runbook/session-service.ts |
— | ⏭️ not mutated |
✅ core — changed-scope mutants (floor 70% shown as score context)
| File | Score | Status |
|---|---|---|
src/testing/session-fixtures.ts |
— | ⏭️ not mutated |
Deploying rundown with
|
| Latest commit: |
3336f24
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6afac62.rundown-7hl.pages.dev |
| Branch Preview URL: | https://784-positional-pop-review-co.rundown-7hl.pages.dev |
tobyhede
changed the title
1 participant
Follow-up to #784, whose review findings arrived after it merged.
What this does NOT do anymore
The first revision of this PR raised the
positional-pop-to-fixtureschangeset frompatchtomajor, on the reasoning that removingSessionService.popRunbookis a breaking public-API change. That was wrong to spend a PR on. Rundown is unreleased and has no downstream consumers, so there is nothing for a bump level to protect. That commit is dropped; the changeset is untouched and still readspatch.The last commit here exists so the same finding stops being generated.
1.
releaseFromSessiondoc rationale (session-service.ts)Justified its in-place shape with "composite operations —
releaseRunbooks,popRunbookIfActive— can release several runbooks against one session snapshot".popRunbookIfActivereleases exactly one run, so the substituted name did not support the argument the sentence exists to make. Now attributes the multi-release case toreleaseRunbooks, and states the conditional pop's own reason separately: its single release folds into the transaction that decides whether the run is still the top.2.
popTopOfStackUnverified@returns(session-fixtures.ts)Promised null "when the stack was empty or the release removed nothing". The second arm is unreachable, as the comment inside the body already argues —
topIdis read offdefaultStack, soprojectRunbookReleasealways removes it.Deleting that arm alone would have left a second inaccuracy: null is also correct when the popped run was the last stack entry (the wf3 → wf2 → wf1 unwind in
session-service.test.tshits exactly that). The replacement names both real cases.3. Write down that versions do not matter here
CLAUDE.md already says this for persisted state — "compatibility code for consumers that do not exist" — but never for package versions, so review kept generating semver findings.
Versioning and changesetssection: the bump field does not matter, the changeset prose does, and a finding whose only consequence lands on a hypothetical consumer is out of scope.tone_instructions(global) and as a.changeset/**/*.mdpath instruction that splits the two halves — never comment on the bump level, do review the prose for accuracy against the diff.Verification
Full
verifychain passes apart fromcheck:md, which fails on 44 files under the untracked.agents/and.claude/skill directories — pre-existing and unrelated.testexits 0: parser 630, cli 5780, core 2836..coderabbit.yamlparses and is schema-shaped (22path_instructionsentries); it is not Prettier-managed in this repo (.prettierignoreline 1).