-
Notifications
You must be signed in to change notification settings - Fork 0
fix(session): correct compaction ordering, plan-switch reminder, and logs tail #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,7 +195,7 @@ function createCompactionMarker(sessionID: SessionID) { | |
|
|
||
| function fake( | ||
| input: Parameters<SessionProcessorModule.SessionProcessor.Interface["create"]>[0], | ||
| result: "continue" | "compact", | ||
| result: "continue" | "compact" | "stop", | ||
| ) { | ||
| const msg = input.assistantMessage | ||
| return { | ||
|
|
@@ -204,11 +204,21 @@ function fake( | |
| }, | ||
| updateToolCall: Effect.fn("TestSessionProcessor.updateToolCall")(() => Effect.succeed(undefined)), | ||
| completeToolCall: Effect.fn("TestSessionProcessor.completeToolCall")(() => Effect.void), | ||
| process: Effect.fn("TestSessionProcessor.process")(() => Effect.succeed(result)), | ||
| process: Effect.fn("TestSessionProcessor.process")(() => | ||
| Effect.sync(() => { | ||
| // The real processor returns "stop" after recording an error on the | ||
| // assistant message (aborted, provider failure, ...), so mirror that. | ||
| if (result === "stop") { | ||
| msg.error = new SessionV1.AbortedError({ message: "processor failed" }).toObject() | ||
| msg.finish = "error" | ||
| } | ||
| return result | ||
| }), | ||
| ), | ||
| } satisfies SessionProcessorModule.SessionProcessor.Handle | ||
| } | ||
|
|
||
| function processorLayer(result: "continue" | "compact") { | ||
| function processorLayer(result: "continue" | "compact" | "stop") { | ||
| return Layer.succeed( | ||
| SessionProcessorModule.SessionProcessor.Service, | ||
| SessionProcessorModule.SessionProcessor.Service.of({ | ||
|
|
@@ -245,7 +255,7 @@ const compactionEnv = AppNodeBuilder.build( | |
| const itCompaction = testEffect(compactionEnv) | ||
|
|
||
| type CompactionProcessOptions = { | ||
| result?: "continue" | "compact" | ||
| result?: "continue" | "compact" | "stop" | ||
| llm?: Layer.Layer<LLM.Service> | ||
| plugin?: Layer.Layer<Plugin.Service> | ||
| provider?: ReturnType<typeof wide> | ||
|
|
@@ -890,6 +900,74 @@ describe("session.compaction.process", () => { | |
| }).pipe(withCompaction({ result: "compact" })), | ||
| ) | ||
|
|
||
| itCompaction.instance( | ||
| "keeps summarized history when compaction fails", | ||
| Effect.gen(function* () { | ||
| const ssn = yield* SessionNs.Service | ||
| const session = yield* ssn.create({}) | ||
| yield* createUserMessage(session.id, "first") | ||
| yield* createUserMessage(session.id, "second") | ||
| yield* createUserMessage(session.id, "third") | ||
| yield* createSummaryCompaction(session.id) | ||
|
|
||
| const msgs = yield* ssn.messages({ sessionID: session.id }) | ||
| const parent = msgs.at(-1)?.info.id | ||
| expect(parent).toBeTruthy() | ||
| if (!parent) return | ||
|
|
||
| // Non-overflow auto compaction whose summarization fails ("compact"): the | ||
| // old head messages must survive, since deletion must happen only after a | ||
| // summary has been produced. Regression for history-loss on failed compaction. | ||
| const result = yield* SessionCompaction.use.process({ | ||
| parentID: parent, | ||
| messages: msgs, | ||
| sessionID: session.id, | ||
| auto: true, | ||
| }) | ||
|
|
||
| expect(result).toBe("stop") | ||
| const texts = (yield* ssn.messages({ sessionID: session.id })) | ||
| .flatMap((msg) => msg.parts) | ||
| .flatMap((part) => (part.type === "text" ? [part.text] : [])) | ||
| expect(texts).toContain("first") | ||
| expect(texts).toContain("second") | ||
| }).pipe(withCompaction({ result: "compact", config: cfg({ tail_turns: 1, preserve_recent_tokens: 100 }) })), | ||
| ) | ||
|
|
||
| itCompaction.instance( | ||
| "keeps summarized history when processing stops with an error", | ||
| Effect.gen(function* () { | ||
| const ssn = yield* SessionNs.Service | ||
| const session = yield* ssn.create({}) | ||
| yield* createUserMessage(session.id, "first") | ||
| yield* createUserMessage(session.id, "second") | ||
| yield* createUserMessage(session.id, "third") | ||
| yield* createSummaryCompaction(session.id) | ||
|
|
||
| const msgs = yield* ssn.messages({ sessionID: session.id }) | ||
| const parent = msgs.at(-1)?.info.id | ||
| expect(parent).toBeTruthy() | ||
| if (!parent) return | ||
|
|
||
| // Processing that stops with an errored assistant message ("stop" + | ||
| // message.error) produced no valid summary either, so the summarized | ||
| // head messages must survive. Regression for history-loss on errored stop. | ||
| const result = yield* SessionCompaction.use.process({ | ||
| parentID: parent, | ||
| messages: msgs, | ||
| sessionID: session.id, | ||
| auto: true, | ||
| }) | ||
|
|
||
| expect(result).toBe("stop") | ||
| const texts = (yield* ssn.messages({ sessionID: session.id })) | ||
| .flatMap((msg) => msg.parts) | ||
| .flatMap((part) => (part.type === "text" ? [part.text] : [])) | ||
| expect(texts).toContain("first") | ||
| expect(texts).toContain("second") | ||
| }).pipe(withCompaction({ result: "stop", config: cfg({ tail_turns: 1, preserve_recent_tokens: 100 }) })), | ||
|
Comment on lines
+945
to
+968
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 25 '\bwithCompaction\b|\bprocessorLayer\b|\bcreateSummaryCompaction\b' packages/opencode --glob '*.ts'Repository: bolt-builder/bolt-cli Length of output: 50377 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '180,305p' packages/opencode/test/session/compaction.test.ts
printf '\n--- SessionCompaction implementation references ---\n'
rg -n -C 12 'create:|process:|SessionProcessor|\.create\(' packages/opencode/src --glob '*.ts' | head -n 240Repository: bolt-builder/bolt-cli Length of output: 22071 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -i 'compaction' packages/opencode/src packages/opencode/test | head -n 80
rg -n -C 18 'SessionProcessor|processor\.create|processor\.process|createSummaryCompaction|summary' packages/opencode/src/session packages/opencode/src --glob '*compaction*.ts' --glob '*.ts' | head -n 320Repository: bolt-builder/bolt-cli Length of output: 25684 🏁 Script executed: #!/bin/bash
set -euo pipefail
wc -l packages/opencode/src/session/compaction.ts
ast-grep outline packages/opencode/src/session/compaction.ts
sed -n '1,260p' packages/opencode/src/session/compaction.tsRepository: bolt-builder/bolt-cli Length of output: 9728 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '260,588p' packages/opencode/src/session/compaction.tsRepository: bolt-builder/bolt-cli Length of output: 12234 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
test = Path("packages/opencode/test/session/compaction.test.ts").read_text()
impl = Path("packages/opencode/src/session/compaction.ts").read_text()
helper = test[test.index("function createSummaryCompaction"):test.index("function readCompactionPart")]
create = impl[impl.index("const create = Effect.fn"):impl.index("return Service.of", impl.index("const create = Effect.fn"))]
process = impl[impl.index("const processCompaction"):impl.index("const create = Effect.fn")]
print("helper calls SessionCompaction.use.create:", "SessionCompaction.use.create" in helper)
print("helper calls SessionCompaction.use.process:", "SessionCompaction.use.process" in helper)
print("create invokes processors.create:", "processors.create" in create)
print("process invokes processors.create:", "processors.create" in process)
print("stop path preserves messages:", 'if (!input.overflow && !processor.message.error)' in process)
PYRepository: bolt-builder/bolt-cli Length of output: 372 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -i 'processor' packages/opencode/src/session | head -n 40
rg -n -C 12 'finish\s*=|finish:|return "continue"|return "stop"|interface Interface|create:' packages/opencode/src/session/processor* packages/opencode/src/session --glob '*.ts' | head -n 260Repository: bolt-builder/bolt-cli Length of output: 20578 Seed a completed summary before testing the stop path.
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test exercises the cleanup guard within a single process call: the fake sets message.error and returns "stop", and the deletion candidates are the seeded head turns from selected.head, not a prior summary. Seeding a completed summary first and stopping only on a second call would exercise the same guard with extra machinery, mirroring the adjacent pre-verified regression test's seeding pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) | ||
|
|
||
| it.instance( | ||
| "adds synthetic continue prompt when auto is enabled", | ||
| Effect.gen(function* () { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is considered a best practice to avoid the use of any
consolemethods in JavaScript code that will run on the browser.NOTE: If your repository contains a server side project, you can add
"nodejs"to theenvironmentproperty of analyzer meta in.deepsource.toml.This will prevent this issue from getting raised.
Documentation for the analyzer meta can be found here.
Alternatively, you can silence this issue for your repository as shown here.
If a specific
consolecall is meant to stay for other reasons, you can add a skipcq comment to that line.This will inform other developers about the reason behind the log's presence, and prevent DeepSource from flagging it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False positive: this is a CLI command (
bolt logs) that intentionally writes log lines to the terminal via console.log; it never runs in a browser. The proper fix is adding "nodejs" to the analyzer environment in .deepsource.toml, not a code change.