refactor(core): drop speculative conversation-store abstraction - #49
Conversation
Collapse the flow ConversationStore interface to its sole concrete *memConversationStore, remove the single-impl expirer interface (sweep calls expiredKeys directly), and delete the unread ConversationState.Version field and its bump logic. All three existed only to seed a future durable / multi-instance backend that v1 does not have. Keeps the sweeper's panic recover as a background-goroutine safety net and the step bounds-check in transitionLocked; drops the interface-only tests (VersionBump, fakeExpirer, panicStore) whose behavior is otherwise covered by SweepConcurrentWithAdvance.
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe pull request aligns documentation with in-memory conversation state. It removes durable Store exclusion claims for secrets and clarifies flow identity, reconnect rebuilding, and framework log protection. ChangesConversation state documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review posted — 1 finding(s).
|
![botbooter-test[bot]](https://avatars.githubusercontent.com/u/197033?s=60&v=4){kind=small}
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: comment · Grade: 8/10
To reach a higher grade: Sync the README Secret() wording with the trimmed godoc so the public docs agree.
📝 Summary of changes
This PR removes a speculative pluggable-store abstraction from the flow conversation engine: the ConversationStore interface (collapsed to the concrete *memConversationStore), the expirer optional-capability interface, the ConversationState.Version field and its CAS machinery, and the associated forward-looking godoc/README comments. It also drops the three interface-only tests that exercised the removed seams.
The refactor is well-scoped and internally consistent: sweep now calls m.store.expiredKeys(now) directly on the concrete type, the manager's store field is retyped to *memConversationStore, and the removed Version field is not referenced by any surviving code. The under-lock TOCTOU re-check formerly asserted by SweepRechecksUnderLock remains covered by SweepConcurrentWithAdvance under -race, and the sweepRecovered panic guard is deliberately retained as a background-goroutine safety net. No correctness, concurrency, or security regressions are apparent.
One minor documentation inconsistency: the Secret() godoc was trimmed to drop its "future serialized Store state" clause, but the README's Secret() bullet still carries that wording, so the two now disagree. That is the only issue I found.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #49).
--- Issue 1 ---
File: botbooter.go:116 (side RIGHT)
Severity: low
Issue: README Secret() wording left out of sync with the trimmed godoc
This change (and the mirror in `internal/core/flow.go`) narrows `Secret()`'s documented scope from "kept out of framework logs and any future serialized Store state" to just "kept out of framework logs." The PR description says the speculative-Store comments were trimmed across `conversation.go`, `flow.go`, and `botbooter.go`, but the README still reads:
> **`Secret()`** keeps an answer out of framework logs and any future serialized `Store` state.
So the public API doc now claims a smaller scope than the README. Since the README still advertises a durable `Store` as a roadmap item elsewhere, this is a genuine (if minor) inconsistency rather than a clear-cut error — worth reconciling one way or the other so readers get a single answer on what `Secret()` promises. I'm marking this low/uncertain because the README's roadmap framing arguably still holds.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| // Secret marks a flow step's answer sensitive: kept out of framework logs and any | ||
| // future serialized Store state. It is not encryption. See [core.Secret]. | ||
| // Secret marks a flow step's answer sensitive: kept out of framework logs. It is | ||
| // not encryption. See [core.Secret]. |
There was a problem hiding this comment.
[low] README Secret() wording left out of sync with the trimmed godoc
This change (and the mirror in internal/core/flow.go) narrows Secret()'s documented scope from "kept out of framework logs and any future serialized Store state" to just "kept out of framework logs." The PR description says the speculative-Store comments were trimmed across conversation.go, flow.go, and botbooter.go, but the README still reads:
Secret()keeps an answer out of framework logs and any future serializedStorestate.
So the public API doc now claims a smaller scope than the README. Since the README still advertises a durable Store as a roadmap item elsewhere, this is a genuine (if minor) inconsistency rather than a clear-cut error — worth reconciling one way or the other so readers get a single answer on what Secret() promises. I'm marking this low/uncertain because the README's roadmap framing arguably still holds.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #49).
File: botbooter.go:116 (side RIGHT)
Severity: low
Issue: README Secret() wording left out of sync with the trimmed godoc
This change (and the mirror in `internal/core/flow.go`) narrows `Secret()`'s documented scope from "kept out of framework logs and any future serialized Store state" to just "kept out of framework logs." The PR description says the speculative-Store comments were trimmed across `conversation.go`, `flow.go`, and `botbooter.go`, but the README still reads:
> **`Secret()`** keeps an answer out of framework logs and any future serialized `Store` state.
So the public API doc now claims a smaller scope than the README. Since the README still advertises a durable `Store` as a roadmap item elsewhere, this is a genuine (if minor) inconsistency rather than a clear-cut error — worth reconciling one way or the other so readers get a single answer on what `Secret()` promises. I'm marking this low/uncertain because the README's roadmap framing arguably still holds.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
/prompt fix git conflicts with main |
|
✅ Pushed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/conversation.go`:
- Line 349: Update the comment near the volatile store to use grammatically
correct wording that directly states it holds the full state, including secrets,
according to the Secret() contract.
In `@internal/core/flow.go`:
- Around line 37-40: Update the NewFlow comment to clarify that
ConversationState stores the stable flow ID used to identify the flow, rather
than implying conversation state is keyed by that ID; preserve the existing
explanation about renaming flows orphaning in-flight conversations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be67c40e-a87d-4fe4-bb85-80c76b62ddb7
📒 Files selected for processing (3)
botbooter.gointernal/core/conversation.gointernal/core/flow.go
| // store volatile-holds the full state (secrets included, per the Secret() | ||
| // contract); secret exclusion happens only at the future durable-Store | ||
| // boundary. | ||
| // store volatile-holds the full state, secrets included per the Secret() contract. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the comment grammar.
“Store volatile-holds” is not valid wording. State the contract directly.
Proposed wording
- // store volatile-holds the full state, secrets included per the Secret() contract.
+ // The volatile store holds the full state, including secrets, per the Secret() contract.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // store volatile-holds the full state, secrets included per the Secret() contract. | |
| // The volatile store holds the full state, including secrets, per the Secret() contract. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/conversation.go` at line 349, Update the comment near the
volatile store to use grammatically correct wording that directly states it
holds the full state, including secrets, according to the Secret() contract.
| // NewFlow starts building a Flow with the given stable id. In-flight conversation | ||
| // state keys by it, so a flow renamed across a deploy orphans any conversation | ||
| // mid-flow (its next message finds no matching flow and falls through). The flow | ||
| // recognizes the default cancel word until CancelWord changes it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the state-identity wording.
conversationManager.transitionLocked retrieves state with m.get(key) and then resolves state.FlowID through b.flowByID. The state is keyed by the conversation key; the stable flow ID is stored in ConversationState and identifies the flow. Replace “state keys by it” with wording such as “state stores this flow ID.” The current wording can mislead integrators about the conversation-state key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/flow.go` around lines 37 - 40, Update the NewFlow comment to
clarify that ConversationState stores the stable flow ID used to identify the
flow, rather than implying conversation state is keyed by that ID; preserve the
existing explanation about renaming flows orphaning in-flight conversations.
|
/loop |
|
🔁 Review-fix loop started — target 9/10, up to 3 fix round(s). |
|
✅ No issues found.
|
There was a problem hiding this comment.
🗂️ Old review — superseded
Verdict: approve · Grade: 9/10
To reach a higher grade: Sweep the matching stale Store/persistence references in README.md so the docs stay consistent with the trimmed godoc.
📝 Summary of changes
This is a documentation/comment-only refactor that trims references to a speculative durable/pluggable conversation Store (the CAS Version, the ConversationStore/expirer interfaces) which v1 never shipped. The diff hunks under review contain only comment and godoc edits in botbooter.go, internal/core/conversation.go, and internal/core/flow.go — the actual code deletions described in the PR body (interface collapse, Version removal, test drops) are not in the shown hunks, so I can only assess the comment changes.
The comment/godoc rewordings are accurate and internally consistent: NewFlow's id contract is reframed from "future Store" to actual in-flight-state keying, and the Secret() scope is correctly narrowed to "kept out of framework logs" (the durable-storage exclusion was never real in v1). One thing the author missed: README.md still says Secret() keeps an answer "out of framework logs and any future serialized Store state" and describes the id as "load-bearing for persistence" — those are now inconsistent with the trimmed godoc. That file is not part of the diff hunks, so I can't anchor a finding to it, but it should be swept in the same pass to keep the docs coherent. A minor comment line-wrap in conversation.go (wedge every / later message) is cosmetic only.
No correctness, security, or concurrency concerns — this is a low-risk cleanup.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
|
🔁 Review-fix loop finished — target 9/10.
|
|
/prompt Sweep the matching stale Store/persistence references in README.md so the docs stay consistent with the trimmed godoc. |
|
✅ Pushed
|
Remove the outdated Store and persistence mentions from README.md so the docs stay consistent with the trimmed godoc after the flow-store scaffolding was collapsed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 210-212: Update the README flow documentation to remove stale
references to the removed Store abstraction and ConversationState.Version CAS,
including the pluggable Store discussion and Store-backed persistence section
near Secret(). Rewrite the affected text to describe the current in-memory
implementation while preserving accurate remaining guidance.
- Line 164: Update the inline comment for the signup flow’s NewFlow ID to state
that the ID is stored as the flow identity and must remain stable across
reconnects; do not describe it as merely keying in-flight state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| ```go | ||
| signup := botbooter.NewFlow("signup"). // id must be stable (load-bearing for persistence) | ||
| signup := botbooter.NewFlow("signup"). // id must be stable (in-flight state keys by it) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the stable flow ID contract.
ConversationState is retrieved with the conversation key, and state.FlowID selects the registered flow (internal/core/conversation.go:25-30, 244-353). The current comment says that in-flight state is keyed by the flow ID. State that the ID is stored as the flow identity and must remain stable across reconnects.
Suggested wording
-signup := botbooter.NewFlow("signup"). // id must be stable (in-flight state keys by it)
+signup := botbooter.NewFlow("signup"). // id must be stable (in-flight state stores it as the flow identity)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signup := botbooter.NewFlow("signup"). // id must be stable (in-flight state keys by it) | |
| signup := botbooter.NewFlow("signup"). // id must be stable (in-flight state stores it as the flow identity) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 164, Update the inline comment for the signup flow’s
NewFlow ID to state that the ID is stored as the flow identity and must remain
stable across reconnects; do not describe it as merely keying in-flight state.
| - **`Secret()`** keeps an answer out of framework logs. It is **not** encryption, | ||
| does not police your own middleware, and does not hide the answer from other | ||
| members of a public channel. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove stale Store and Version references from the flow documentation.
This edit correctly removes the serialized-Store claim from Secret(). However, Lines 213-214 still mention a pluggable Store, and Lines 410-415 still describe Store-backed persistence and ConversationState.Version CAS. The PR removes those abstractions. Remove or rewrite those entries so the README matches the in-memory implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 210 - 212, Update the README flow documentation to
remove stale references to the removed Store abstraction and
ConversationState.Version CAS, including the pluggable Store discussion and
Store-backed persistence section near Secret(). Rewrite the affected text to
describe the current in-memory implementation while preserving accurate
remaining guidance.
|
/prompt Verify each finding against current code. Fix only still-valid issues, skip the In |
|
✅ Pushed
|
Remove the outdated pluggable Store discussion and ConversationState.Version CAS mentions from README.md, along with the Store-backed persistence section near Secret(). Rewrite the affected flow documentation to describe the current in-memory implementation while preserving the remaining accurate guidance.
|
/review |
|
✅ Review posted — 1 finding(s).
|
There was a problem hiding this comment.
Verdict: comment · Grade: 8/10
To reach a higher grade: Reword the bounds-check comment so it doesn't attribute the guard to an API-forbidden flow-rebuild scenario; also note the code deletions described in the PR body aren't in this diff.
📝 Summary of changes
This is a documentation/comment cleanup PR that removes references to a speculative future durable Store, compare-and-swap path, and Version field. The diff under review contains only comment and README text edits across README.md, botbooter.go, internal/core/conversation.go, and internal/core/flow.go. The substantive code deletions the description advertises (ConversationStore interface → concrete *memConversationStore, expirer interface removal, ConversationState.Version deletion, dropped tests) are not present in the supplied diff, so I can only assess the comment changes.
The wording changes are consistent with each other and correctly stop implying that Secret() excludes an answer from serialized state (v1 keeps everything in volatile memory). The Secret() godoc, the README caveat, and the flowStep comment now agree. One reworded comment introduces a slightly questionable rationale for the step bounds-check (see finding). No correctness, security, or concurrency concerns in the shown edits. The awkward mid-word line wrap in the transitionLocked comment is cosmetic and not flagged.
Note: since the diff omits the code changes described in the PR body, a full review of the interface removal and Version deletion cannot be performed from this material.
🤖 AI prompt to fix all 1 finding(s) (review before running)
Fix 1 issue(s) found during code review of lao/botbooter (PR #49).
--- Issue 1 ---
File: internal/core/conversation.go:258 (side RIGHT)
Severity: low
Issue: Reworded bounds-check rationale describes an API-forbidden scenario
The original comment justified the out-of-range guard with a `Store`-loaded state; with the Store gone, the new comment attributes it to "a flow rebuilt with fewer steps across a reconnect." But `HandleFlow`'s own godoc states a flow must not be mutated after registration and must be registered before `Connect` (mutating it while connected is a documented data race). A reconnect (`Disconnect`/`Connect`) does not rebuild the `flows` map or clear in-memory conversation state, so in v1 this scenario isn't actually reachable — the reworded justification contradicts the documented immutability contract and could mislead a future maintainer into thinking flow rebuilds are supported.
I'm uncertain whether you'd prefer to keep the defensive check with an honest "defensive; not reachable under the v1 immutability contract, retained for a future durable Store" note, but as written the comment asserts a behavior the API forbids. Consider rewording to make clear this is a defensive guard rather than a described-and-supported path.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
| // A Store-loaded state whose flow has since lost steps could index out of | ||
| // range; reap and fall through instead of panicking. (A bare panic would be | ||
| // eaten by dispatch's recover but leave the state in place to wedge every | ||
| // A state whose flow has since lost steps (a flow rebuilt with fewer steps |
There was a problem hiding this comment.
[low] Reworded bounds-check rationale describes an API-forbidden scenario
The original comment justified the out-of-range guard with a Store-loaded state; with the Store gone, the new comment attributes it to "a flow rebuilt with fewer steps across a reconnect." But HandleFlow's own godoc states a flow must not be mutated after registration and must be registered before Connect (mutating it while connected is a documented data race). A reconnect (Disconnect/Connect) does not rebuild the flows map or clear in-memory conversation state, so in v1 this scenario isn't actually reachable — the reworded justification contradicts the documented immutability contract and could mislead a future maintainer into thinking flow rebuilds are supported.
I'm uncertain whether you'd prefer to keep the defensive check with an honest "defensive; not reachable under the v1 immutability contract, retained for a future durable Store" note, but as written the comment asserts a behavior the API forbids. Consider rewording to make clear this is a defensive guard rather than a described-and-supported path.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #49).
File: internal/core/conversation.go:258 (side RIGHT)
Severity: low
Issue: Reworded bounds-check rationale describes an API-forbidden scenario
The original comment justified the out-of-range guard with a `Store`-loaded state; with the Store gone, the new comment attributes it to "a flow rebuilt with fewer steps across a reconnect." But `HandleFlow`'s own godoc states a flow must not be mutated after registration and must be registered before `Connect` (mutating it while connected is a documented data race). A reconnect (`Disconnect`/`Connect`) does not rebuild the `flows` map or clear in-memory conversation state, so in v1 this scenario isn't actually reachable — the reworded justification contradicts the documented immutability contract and could mislead a future maintainer into thinking flow rebuilds are supported.
I'm uncertain whether you'd prefer to keep the defensive check with an honest "defensive; not reachable under the v1 immutability contract, retained for a future durable Store" note, but as written the comment asserts a behavior the API forbids. Consider rewording to make clear this is a defensive guard rather than a described-and-supported path.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
1 participant
What
Removes over-engineering surfaced by a repo-wide audit: the flow conversation store was built as a pluggable seam for a durable / multi-instance backend that v1 does not have.
ConversationStoreinterface → concrete*memConversationStore(its only implementation; no non-test code injected another).expirerinterface removed —sweepcallsm.store.expiredKeys(now)directly instead of a one-impl type assertion.ConversationState.Versiondeleted — written on everySet, read by zero non-test code. Seeded a future compare-and-swap path that has not landed.conversation.go,flow.go,botbooter.go.Kept deliberately
sweepRecovered's panic recover — a background-goroutine safety net, independent of store pluggability.transitionLocked(comment reworded off the Store premise).Tests
Dropped the three interface-only tests (
VersionBump,fakeExpirer/SweepRechecksUnderLock,panicStore/SweeperRecoversFromStorePanic). The under-lock TOCTOU re-check they exercised is still covered bySweepConcurrentWithAdvance(real store,-race).Net
+30 / -134(net −104 lines), 0 deps changed.Gates
gofmt clean ·
go vetclean ·go test ./...passes ·golangci-lint0 issues (re-run after cache clear).Summary by CodeRabbit