From b3ec68df26736f3de6070ce27d6a621e523f7cf9 Mon Sep 17 00:00:00 2001 From: Linus Date: Mon, 21 Sep 2026 18:00:13 +0200 Subject: [PATCH 1/2] Keep a team-mode area open for the manager's next message Seen live: the buddy drafted an answer to an escalation, the manager said "You can send it", and no confirm button appeared. The buddy claimed one existed and then denied having any such tool. Nothing was stored. Team mode mounts an area's tools only in the turn that opened it, the set of opened areas was rebuilt empty on every message, and the stored transcript is text only. So by the time the manager approved a draft, the tool that makes the change was gone and the model had nothing to call, and invented the confirmation. This hit every "discuss it, then approve it" flow, not just answering escalations. A reply now stores the areas it opened (buddy_team_messages.opened_areas, nullable), and the next message mounts them from its first hop. Only what the reply itself opened is stored, not what it inherited, so an area stays open for one further message and a conversation does not slowly mount every area's tools, which is the reason areas exist. A fold of old messages into the memory note does not close it, because the last reply is read from the whole transcript. Also describes each area in open_area's definition. The model chose an area from its name alone and opened arrival when asked about questions hires were waiting on, because "knowledge" says nothing. The definition now lists what is in each area, says an area stays open for one more message, and forbids saying something was offered for confirmation unless a tool of an opened area did it. The column is nullable, so Hibernate's ddl-auto update can add it to a populated table; V20 does the same by hand, idempotently. Co-Authored-By: Claude Sonnet 5 --- .../model/entity/BuddyTeamMessage.kt | 10 ++ .../onboarding/service/BuddyTeamService.kt | 35 ++-- .../onboarding/service/BuddyTeamTools.kt | 6 +- .../onboarding/service/OpenAreas.kt | 40 +++++ .../onboarding/service/TeamAreaTools.kt | 26 ++- ...0__add_buddy_team_message_opened_areas.sql | 5 + .../BuddyTeamMessageRepositoryTest.kt | 56 +++++++ .../service/BuddyTeamServiceTest.kt | 157 ++++++++++++++++++ .../onboarding/service/BuddyTeamToolsTest.kt | 31 ++++ .../onboarding/service/OpenAreasTest.kt | 52 ++++++ 10 files changed, 398 insertions(+), 20 deletions(-) create mode 100644 src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt create mode 100644 src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql create mode 100644 src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/repository/BuddyTeamMessageRepositoryTest.kt create mode 100644 src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreasTest.kt diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt index 2417f30c..f681d90a 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt @@ -35,4 +35,14 @@ class BuddyTeamMessage( val createdAt: Instant = Instant.now(), @Column(nullable = false) val opening: Boolean = false, + /** + * The team areas this reply opened, as a comma-separated list of area names; null when it opened + * none, and for every message written before the column existed. + * + * Read back on the manager's *next* message so that an area opened to draft something is still + * open when they say "yes, send it". The transcript holds text only, so without this the tools that + * make the change are gone by the time the manager approves, and the model has nothing to call. + */ + @Column(name = "opened_areas", nullable = true) + val openedAreas: String? = null, ) diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt index 0b5946d6..21184da3 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt @@ -144,10 +144,15 @@ class BuddyTeamService( val session = getOrCreateSession(userId, projectId) // Read before saving the new message so it is not sent to the AI twice. - val history = buddyTeamMessageRepository - .findAllBySessionIdOrderByCreatedAtAsc(session.id) - .drop(session.summarizedCount) - .map { it.toAgentMessage() } + val transcript = buddyTeamMessageRepository.findAllBySessionIdOrderByCreatedAtAsc(session.id) + val history = transcript.drop(session.summarizedCount).map { it.toAgentMessage() } + // The last reply is the one this message answers, so it decides what is still open. It is the whole + // transcript, not the unfolded part: a fold must not close what the last reply opened. + val carriedOver = transcript + .lastOrNull() + ?.takeIf { it.role == BuddyMessageRole.ASSISTANT } + ?.openedAreas + .toTeamAreas() buddyTeamMessageRepository.save( BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = content), @@ -160,12 +165,13 @@ class BuddyTeamService( var citations: List = emptyList() var answer: String? = null var step = 0 - val openedAreas = mutableSetOf() + val areas = OpenAreas(carriedOver) while (answer == null && step < BuddyService.MAX_AGENT_STEPS) { step++ - // Per hop, not per turn: an area opened on the previous hop is mounted from this one. - val tools = if (capabilitiesEnabled) buddyTeamTools.toolSpecs(openedAreas) else emptyList() + // Per hop, not per turn: an area opened on the previous hop is mounted from this one, and + // one opened by the previous *reply* is mounted from the first. + val tools = if (capabilitiesEnabled) buddyTeamTools.toolSpecs(areas.mounted) else emptyList() val response = onboardingAiClient.buddyAgentTurn( BuddyAgentRequest( messages = messages, @@ -188,7 +194,7 @@ class BuddyTeamService( next.add( BuddyAgentMessageDto( role = "tool", - content = runToolCall(call, context, mounted, openedAreas), + content = runToolCall(call, context, mounted, areas), toolCallId = call.id, ), ) @@ -201,7 +207,14 @@ class BuddyTeamService( emitAgentReply(reply, citations) buddyTeamMessageRepository.save( - BuddyTeamMessage(session = session, role = BuddyMessageRole.ASSISTANT, content = reply), + BuddyTeamMessage( + session = session, + role = BuddyMessageRole.ASSISTANT, + content = reply, + // Only what this reply opened, not what it inherited: an area stays open for one more + // message, so a conversation does not slowly mount every area's tools. + openedAreas = areas.openedThisTurn.encoded(), + ), ) compactInBackground(userId, projectId) } @@ -219,7 +232,7 @@ class BuddyTeamService( call: BuddyToolCallDto, context: TeamToolContext, mountedToolNames: Set, - openedAreas: MutableSet, + areas: OpenAreas, ): String { if (call.name in mountedToolNames && buddyProposalService.isAction(call.name)) { val outcome = buddyProposalService.propose(call, context) @@ -240,7 +253,7 @@ class BuddyTeamService( emit(BuddyStreamEvent(type = "tool_use", name = call.name, kind = "tool")) if (call.name == BuddyTeamTools.OPEN_AREA && call.name in mountedToolNames) { val outcome = buddyTeamTools.openArea(call) - outcome.area?.let { openedAreas.add(it) } + outcome.area?.let { areas.open(it) } return outcome.toolResult } return buddyTeamTools.execute(call, context, mountedToolNames) diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt index aa8aa594..6e9b730c 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt @@ -279,7 +279,11 @@ class BuddyTeamTools( name = OPEN_AREA, description = "Open one area of the manager's work so its tools become available on your next " + "step. Open an area only when the manager asks about something in it; its tools are not " + - "available until you have opened it.", + "available until you have opened it. An area stays open for the manager's next message too, " + + "and no longer: if they ask you to act on something from earlier and the tool is not there, " + + "open the area again first. Never say something has been offered for confirmation unless a " + + "tool of an opened area did it.\n\nThe areas:\n" + + openableAreas().sorted().joinToString("\n") { "- ${it.name.lowercase()}: ${it.summary}" }, parameters = buildJsonObject { put("type", "object") putJsonObject("properties") { diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt new file mode 100644 index 00000000..b7a0bb62 --- /dev/null +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt @@ -0,0 +1,40 @@ +package com.sprintstart.sprintstartbackend.onboarding.service + +/** + * The team areas whose tools are mounted while one manager message is answered. + * + * Two things put an area here. The manager's previous message may have opened it ([carriedOver]), which is + * what makes "yes, send it" work: drafting something opens an area, approving it comes a message later, + * and the transcript is text only, so nothing else remembers that the area was open. And the model can open + * one itself during this turn ([open]). + * + * Only what this turn opened is remembered for the next one ([openedThisTurn]). What was carried over is not + * carried again, so an area stays open for one further message and a long conversation does not slowly + * mount every area's tools — the reason areas exist. + */ +internal class OpenAreas( + carriedOver: Set = emptySet(), +) { + /** Every area mounted so far, this turn's and the last one's. Read again on each hop. */ + val mounted: MutableSet = carriedOver.toMutableSet() + + /** The areas the model opened during this turn. */ + val openedThisTurn: MutableSet = mutableSetOf() + + fun open(area: TeamArea) { + mounted.add(area) + openedThisTurn.add(area) + } +} + +/** The stored form of the areas a reply opened: their names, comma-separated and sorted; null when none. */ +internal fun Set.encoded(): String? = + takeIf { it.isNotEmpty() }?.sortedBy { it.name }?.joinToString(",") { it.name } + +/** The areas a stored [encoded] value names. A name that is no longer an area is dropped, never an error. */ +internal fun String?.toTeamAreas(): Set = + this + ?.split(',') + ?.mapNotNull { name -> TeamArea.entries.firstOrNull { it.name == name.trim() } } + ?.toSet() + .orEmpty() diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/TeamAreaTools.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/TeamAreaTools.kt index cdb705f1..330e8845 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/TeamAreaTools.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/TeamAreaTools.kt @@ -4,14 +4,24 @@ import com.sprintstart.sprintstartbackend.onboarding.external.model.BuddyToolCal import com.sprintstart.sprintstartbackend.onboarding.external.model.BuddyToolSpecDto import java.util.UUID -/** A part of the manager's surface the team-mode buddy can open, each with its own tools. */ -enum class TeamArea { - KNOWLEDGE, - STARTER_WORK, - TEAM, - ARRIVAL, - CONTENT, - SOURCES, +/** + * A part of the manager's surface the team-mode buddy can open, each with its own tools. + * + * @property summary What is inside, in the manager's words. The model chooses an area from its name and + * this alone, and a name is not enough: "knowledge" does not say that hires' escalated questions live there. + */ +enum class TeamArea( + val summary: String, +) { + KNOWLEDGE("questions hires escalated because nobody could answer them, and the canonical answers written for them"), + STARTER_WORK("the pool of starter-work tasks hires are offered, and GitHub issues that could join it"), + TEAM("who is on the project and which roles they hold: adding and removing people, giving and taking roles"), + ARRIVAL("what has to be true before a new hire can start working, as a list the project owns"), + CONTENT( + "the onboarding paths of the project's members: their phases, steps, tasks and links, skip requests, " + + "feedback, knowledge checks and orientation packets", + ), + SOURCES("where the project's material comes from: repositories, other connected sources and uploads"), } /** Who a team-mode tool runs for, and on which project. Resolved and authorised before any tool runs. */ diff --git a/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql b/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql new file mode 100644 index 00000000..60dcd77c --- /dev/null +++ b/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql @@ -0,0 +1,5 @@ +-- The team areas a buddy reply opened, so the manager's next message still has the tools that area mounts. +-- Nullable on purpose: existing messages opened nothing that matters, and Hibernate (ddl-auto: update) can add +-- a nullable column to a populated table, which it cannot do for NOT NULL ones. Idempotent. +ALTER TABLE buddy_team_messages + ADD COLUMN IF NOT EXISTS opened_areas VARCHAR(255); diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/repository/BuddyTeamMessageRepositoryTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/repository/BuddyTeamMessageRepositoryTest.kt new file mode 100644 index 00000000..6424ef14 --- /dev/null +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/repository/BuddyTeamMessageRepositoryTest.kt @@ -0,0 +1,56 @@ +package com.sprintstart.sprintstartbackend.onboarding.repository + +import com.sprintstart.sprintstartbackend.onboarding.external.enums.BuddyMessageRole +import com.sprintstart.sprintstartbackend.onboarding.model.entity.BuddyTeamMessage +import com.sprintstart.sprintstartbackend.onboarding.model.entity.BuddyTeamSession +import com.sprintstart.sprintstartbackend.shared.crypto.CryptoConfiguration +import jakarta.persistence.EntityManager +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.boot.data.jpa.test.autoconfigure.DataJpaTest +import org.springframework.context.annotation.Import +import org.springframework.test.context.ActiveProfiles +import java.time.Instant +import java.util.UUID + +/** + * What a team reply opened has to survive the trip to the database and back: the next message reads it to + * decide which tools to mount, and a value that came back changed or empty would silently bring the + * "yes, send it" failure back. + */ +@ActiveProfiles("test") +@DataJpaTest +@Import(CryptoConfiguration::class) +class BuddyTeamMessageRepositoryTest { + @Autowired + private lateinit var repository: BuddyTeamMessageRepository + + @Autowired + private lateinit var entityManager: EntityManager + + private fun session(): BuddyTeamSession = + BuddyTeamSession(userId = UUID.randomUUID(), projectId = UUID.randomUUID()).also { entityManager.persist(it) } + + private fun message(session: BuddyTeamSession, offsetSeconds: Long, opened: String?) = + BuddyTeamMessage( + session = session, + role = BuddyMessageRole.ASSISTANT, + content = "reply $offsetSeconds", + createdAt = Instant.parse("2026-09-21T10:00:00Z").plusSeconds(offsetSeconds), + openedAreas = opened, + ) + + @Test + fun `the areas a reply opened come back exactly as stored, and none comes back as null`() { + val session = session() + repository.save(message(session, 1, "ARRIVAL,KNOWLEDGE")) + repository.save(message(session, 2, null)) + entityManager.flush() + entityManager.clear() + + val stored = repository.findAllBySessionIdOrderByCreatedAtAsc(session.id) + + assertThat(stored.map { it.openedAreas }).containsExactly("ARRIVAL,KNOWLEDGE", null) + } +} diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt index 5a1e6795..3a840ddd 100644 --- a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt @@ -195,6 +195,163 @@ class BuddyTeamServiceTest { assertThat(mountedSets).containsExactly(emptySet(), setOf(TeamArea.KNOWLEDGE)) } + /** + * The failure this guards against, seen live: the buddy drafts an answer (opening the knowledge area), + * the manager says "yes, send it" in the next message, and the tool that makes the change is gone, so the + * model invents a confirm button. The transcript is text only; what a reply opened is stored with it. + */ + private fun asTranscript(vararg messages: BuddyTeamMessage) { + every { buddyTeamMessageRepository.findAllBySessionIdOrderByCreatedAtAsc(session.id) } returns + messages.toList() + } + + private fun reply(content: String = "Here is a draft.", opened: String? = null, opening: Boolean = false) = + BuddyTeamMessage( + session = session, + role = BuddyMessageRole.ASSISTANT, + content = content, + opening = opening, + openedAreas = opened, + ) + + private fun mountedOnEachHop(): MutableList> { + val mountedSets = mutableListOf>() + every { buddyTeamTools.toolSpecs(any()) } answers { + mountedSets.add(firstArg>().toSet()) + listOf(spec(BuddyTeamTools.OPEN_AREA)) + } + return mountedSets + } + + private fun savedReplies(): List { + val saved = mutableListOf() + every { buddyTeamMessageRepository.save(any()) } answers { + firstArg().also { saved.add(it) } + } + return saved + } + + @Test + fun `an area the last reply opened is still mounted when the manager approves what it drafted`() = runTest { + asTranscript( + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "can you answer Ada?"), + reply(opened = "KNOWLEDGE"), + ) + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Done.") + + service.sendMessageForMe(authId, projectId, "You can send it").toList() + + assertThat(mountedSets).containsExactly(setOf(TeamArea.KNOWLEDGE)) + } + + @Test + fun `stores the areas a reply opened with the reply`() = runTest { + val saved = savedReplies() + val mountedSets = mountedOnEachHop() + every { buddyTeamTools.openArea(any()) } returns + BuddyTeamTools.OpenAreaOutcome(area = TeamArea.KNOWLEDGE, toolResult = "Opened knowledge.") + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returnsMany listOf( + BuddyAgentResponse( + final = false, + messages = listOf(BuddyAgentMessageDto(role = "assistant")), + pendingToolCalls = listOf( + BuddyToolCallDto( + id = "c1", + name = BuddyTeamTools.OPEN_AREA, + arguments = JsonObject(mapOf("area" to JsonPrimitive("knowledge"))), + ), + ), + ), + finalReply("Here is a draft."), + ) + + service.sendMessageForMe(authId, projectId, "can you answer Ada?").toList() + + assertThat(mountedSets).containsExactly(emptySet(), setOf(TeamArea.KNOWLEDGE)) + assertThat(saved.single { it.role == BuddyMessageRole.ASSISTANT }.openedAreas).isEqualTo("KNOWLEDGE") + } + + @Test + fun `an area that was only carried over is not carried again`() = runTest { + asTranscript(reply(opened = "KNOWLEDGE")) + val saved = savedReplies() + mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Sent.") + + service.sendMessageForMe(authId, projectId, "You can send it").toList() + + assertThat(saved.single { it.role == BuddyMessageRole.ASSISTANT }.openedAreas).isNull() + } + + @Test + fun `opening the area again keeps it open for one more message`() = runTest { + asTranscript(reply(opened = "KNOWLEDGE")) + val saved = savedReplies() + mountedOnEachHop() + every { buddyTeamTools.openArea(any()) } returns + BuddyTeamTools.OpenAreaOutcome(area = TeamArea.KNOWLEDGE, toolResult = "Opened knowledge.") + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returnsMany listOf( + BuddyAgentResponse( + final = false, + messages = listOf(BuddyAgentMessageDto(role = "assistant")), + pendingToolCalls = listOf( + BuddyToolCallDto( + id = "c1", + name = BuddyTeamTools.OPEN_AREA, + arguments = JsonObject(mapOf("area" to JsonPrimitive("knowledge"))), + ), + ), + ), + finalReply("Anything else in there?"), + ) + + service.sendMessageForMe(authId, projectId, "and the other one?").toList() + + assertThat(saved.single { it.role == BuddyMessageRole.ASSISTANT }.openedAreas).isEqualTo("KNOWLEDGE") + } + + @Test + fun `nothing is carried over from anything but the reply just before`() = runTest { + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Ok.") + + // An older reply opened an area, but a later one did not: the area has closed. + asTranscript( + reply(opened = "KNOWLEDGE"), + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "thanks"), + reply(content = "You are welcome."), + ) + service.sendMessageForMe(authId, projectId, "and now?").toList() + + // The last message is a greeting that opens a visit. + asTranscript(reply(content = "Hi again.", opening = true)) + service.sendMessageForMe(authId, projectId, "hello").toList() + + // The last message is the manager's own, so there is no reply to inherit from. + asTranscript(BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "hello?")) + service.sendMessageForMe(authId, projectId, "anyone there?").toList() + + assertThat(mountedSets).containsExactly(emptySet(), emptySet(), emptySet()) + } + + @Test + fun `folding old messages into the memory note does not close what the last reply opened`() = runTest { + session.summarizedCount = 2 + asTranscript( + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "old question"), + reply(content = "old answer"), + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "can you answer Ada?"), + reply(opened = "KNOWLEDGE"), + ) + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Done.") + + service.sendMessageForMe(authId, projectId, "You can send it").toList() + + assertThat(mountedSets).containsExactly(setOf(TeamArea.KNOWLEDGE)) + } + @Test fun `capabilities off mounts no tools in team mode either`() = runTest { val requests = mutableListOf() diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt index 1b64851a..b3f86602 100644 --- a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt @@ -374,4 +374,35 @@ class BuddyTeamToolsTest { assertThat(offered.map { it.jsonPrimitive.content }).containsExactly("knowledge") } + + /** + * The model picks an area from what the definition says is in it. Seen live: asked about questions hires + * were waiting on an answer for, it opened the arrival area, because "knowledge" said nothing. + */ + @Test + fun `open_area's definition says what is in each area it offers, and only those`() { + val tools = tools( + actions = listOf( + action("answer_escalation", TeamArea.KNOWLEDGE), + action("create_arrival_steps", TeamArea.ARRIVAL), + ), + ) + + val description = tools.toolSpecs(emptySet()).single { it.name == BuddyTeamTools.OPEN_AREA }.description + + assertThat(description).contains("- knowledge: ${TeamArea.KNOWLEDGE.summary}") + assertThat(description).contains("- arrival: ${TeamArea.ARRIVAL.summary}") + assertThat(description).doesNotContain("- starter_work:").doesNotContain("- content:") + } + + @Test + fun `open_area's definition says an area stays open for one more message, and forbids inventing a confirmation`() { + val tools = tools(actions = listOf(action("answer_escalation", TeamArea.KNOWLEDGE))) + + val description = tools.toolSpecs(emptySet()).single { it.name == BuddyTeamTools.OPEN_AREA }.description + + assertThat(description).contains("stays open for the manager's next message too") + assertThat(description).contains("open the area again first") + assertThat(description).contains("Never say something has been offered for confirmation unless a tool") + } } diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreasTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreasTest.kt new file mode 100644 index 00000000..9c19f1df --- /dev/null +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreasTest.kt @@ -0,0 +1,52 @@ +package com.sprintstart.sprintstartbackend.onboarding.service + +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test + +class OpenAreasTest { + @Test + fun `an area that was carried over is mounted but not recorded as opened this turn`() { + val areas = OpenAreas(carriedOver = setOf(TeamArea.KNOWLEDGE)) + + assertThat(areas.mounted).containsExactly(TeamArea.KNOWLEDGE) + assertThat(areas.openedThisTurn).isEmpty() + } + + @Test + fun `opening an area mounts it and records it, whether or not it was already there`() { + val areas = OpenAreas(carriedOver = setOf(TeamArea.KNOWLEDGE)) + + areas.open(TeamArea.KNOWLEDGE) + areas.open(TeamArea.ARRIVAL) + + assertThat(areas.mounted).containsExactlyInAnyOrder(TeamArea.KNOWLEDGE, TeamArea.ARRIVAL) + assertThat(areas.openedThisTurn).containsExactlyInAnyOrder(TeamArea.KNOWLEDGE, TeamArea.ARRIVAL) + } + + @Test + fun `areas are stored by name, sorted, and read back`() { + val stored = setOf(TeamArea.STARTER_WORK, TeamArea.ARRIVAL).encoded() + + assertThat(stored).isEqualTo("ARRIVAL,STARTER_WORK") + assertThat(stored.toTeamAreas()).containsExactlyInAnyOrder(TeamArea.ARRIVAL, TeamArea.STARTER_WORK) + } + + @Test + fun `no areas is stored as nothing, and nothing reads back as no areas`() { + assertThat(emptySet().encoded()).isNull() + assertThat(null.toTeamAreas()).isEmpty() + assertThat("".toTeamAreas()).isEmpty() + } + + @Test + fun `a stored name that is no longer an area is dropped rather than failing the turn`() { + assertThat("KNOWLEDGE, NOT_AN_AREA ,".toTeamAreas()).containsExactly(TeamArea.KNOWLEDGE) + } + + @Test + fun `every area has a summary the model can choose it by`() { + TeamArea.entries.forEach { + assertThat(it.summary).describedAs(it.name).isNotBlank() + } + } +} From 4216cb606e209406d730e6172d4ec9310abad266 Mon Sep 17 00:00:00 2001 From: Linus Date: Mon, 28 Sep 2026 14:34:17 +0200 Subject: [PATCH 2/2] Keep team-mode areas open for the whole visit, and never show a tool call as a reply An area opened earlier in the visit is now mounted from the first hop of every later message, not only the one right after it. "Draft it", "make it shorter", "send it" lost the tools at the third message before. A visit begins at the last greeting, as the manager sees it, so a new one starts with nothing mounted. Nothing new is stored: the set is read back from each reply's opened_areas. A final answer that is a tool call written out as text ({"name": ..., "parameters": ...}) is no longer treated as an answer. It is sent back to the model once per hop, and when the step budget runs out the manager gets the fallback reply, never the raw call. Co-Authored-By: Claude Sonnet 5 --- .../model/entity/BuddyTeamMessage.kt | 7 +- .../onboarding/service/BuddyTeamService.kt | 24 ++--- .../onboarding/service/BuddyTeamTools.kt | 6 +- .../onboarding/service/OpenAreas.kt | 29 ++++-- .../onboarding/service/WrittenOutToolCall.kt | 20 ++++ ...0__add_buddy_team_message_opened_areas.sql | 2 +- .../service/BuddyTeamServiceTest.kt | 97 ++++++++++++++++--- .../onboarding/service/BuddyTeamToolsTest.kt | 6 +- .../service/WrittenOutToolCallTest.kt | 23 +++++ 9 files changed, 173 insertions(+), 41 deletions(-) create mode 100644 src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCall.kt create mode 100644 src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCallTest.kt diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt index f681d90a..85edd8d1 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/model/entity/BuddyTeamMessage.kt @@ -39,9 +39,10 @@ class BuddyTeamMessage( * The team areas this reply opened, as a comma-separated list of area names; null when it opened * none, and for every message written before the column existed. * - * Read back on the manager's *next* message so that an area opened to draft something is still - * open when they say "yes, send it". The transcript holds text only, so without this the tools that - * make the change are gone by the time the manager approves, and the model has nothing to call. + * Read back on the manager's later messages of the same visit so that an area opened to draft + * something is still open when they say "yes, send it". The transcript holds text only, so without + * this the tools that make the change are gone by the time the manager approves, and the model has + * nothing to call. */ @Column(name = "opened_areas", nullable = true) val openedAreas: String? = null, diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt index 21184da3..983f67a2 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamService.kt @@ -146,13 +146,9 @@ class BuddyTeamService( // Read before saving the new message so it is not sent to the AI twice. val transcript = buddyTeamMessageRepository.findAllBySessionIdOrderByCreatedAtAsc(session.id) val history = transcript.drop(session.summarizedCount).map { it.toAgentMessage() } - // The last reply is the one this message answers, so it decides what is still open. It is the whole - // transcript, not the unfolded part: a fold must not close what the last reply opened. - val carriedOver = transcript - .lastOrNull() - ?.takeIf { it.role == BuddyMessageRole.ASSISTANT } - ?.openedAreas - .toTeamAreas() + // Everything the visit has opened, from the whole transcript rather than the unfolded part: a fold + // must not close an area the manager is still working in. + val carriedOver = transcript.areasOpenedThisVisit() buddyTeamMessageRepository.save( BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = content), @@ -170,7 +166,7 @@ class BuddyTeamService( while (answer == null && step < BuddyService.MAX_AGENT_STEPS) { step++ // Per hop, not per turn: an area opened on the previous hop is mounted from this one, and - // one opened by the previous *reply* is mounted from the first. + // one opened earlier in the visit is mounted from the first. val tools = if (capabilitiesEnabled) buddyTeamTools.toolSpecs(areas.mounted) else emptyList() val response = onboardingAiClient.buddyAgentTurn( BuddyAgentRequest( @@ -185,7 +181,14 @@ class BuddyTeamService( ), ) citations = response.citations - if (response.final) { + if (response.final && response.text.writesOutAToolCall()) { + // Not an answer: shown, it would read as a reply to the manager. Sent back once per hop, + // and if the budget runs out the reply is the fallback, never the raw call. + logger.warn("Team buddy wrote a tool call out as its reply; asking again") + messages = response.messages.ifEmpty { + messages + BuddyAgentMessageDto(role = "assistant", content = response.text) + } + BuddyAgentMessageDto(role = "user", content = TOOL_CALL_WRITTEN_OUT) + } else if (response.final) { answer = response.text } else { val mounted = tools.map { it.name }.toSet() @@ -211,8 +214,7 @@ class BuddyTeamService( session = session, role = BuddyMessageRole.ASSISTANT, content = reply, - // Only what this reply opened, not what it inherited: an area stays open for one more - // message, so a conversation does not slowly mount every area's tools. + // Only what this reply opened; the visit's areas are read back from these. openedAreas = areas.openedThisTurn.encoded(), ), ) diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt index 6e9b730c..bac63242 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamTools.kt @@ -279,9 +279,9 @@ class BuddyTeamTools( name = OPEN_AREA, description = "Open one area of the manager's work so its tools become available on your next " + "step. Open an area only when the manager asks about something in it; its tools are not " + - "available until you have opened it. An area stays open for the manager's next message too, " + - "and no longer: if they ask you to act on something from earlier and the tool is not there, " + - "open the area again first. Never say something has been offered for confirmation unless a " + + "available until you have opened it. An area stays open for the rest of this visit, so you " + + "do not open it again to act on something you discussed. If a tool you need is not there, " + + "open its area first. Never say something has been offered for confirmation unless a " + "tool of an opened area did it.\n\nThe areas:\n" + openableAreas().sorted().joinToString("\n") { "- ${it.name.lowercase()}: ${it.summary}" }, parameters = buildJsonObject { diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt index b7a0bb62..c445f06d 100644 --- a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/OpenAreas.kt @@ -1,16 +1,18 @@ package com.sprintstart.sprintstartbackend.onboarding.service +import com.sprintstart.sprintstartbackend.onboarding.external.enums.BuddyMessageRole +import com.sprintstart.sprintstartbackend.onboarding.model.entity.BuddyTeamMessage + /** * The team areas whose tools are mounted while one manager message is answered. * - * Two things put an area here. The manager's previous message may have opened it ([carriedOver]), which is - * what makes "yes, send it" work: drafting something opens an area, approving it comes a message later, - * and the transcript is text only, so nothing else remembers that the area was open. And the model can open - * one itself during this turn ([open]). + * Two things put an area here. Earlier in the visit a reply may have opened it ([carriedOver]), which is + * what makes "yes, send it" work however long the discussion before it ran: drafting something opens an + * area, and approving it comes messages later, and the transcript is text only, so nothing else remembers + * that the area was open. And the model can open one itself during this turn ([open]). * - * Only what this turn opened is remembered for the next one ([openedThisTurn]). What was carried over is not - * carried again, so an area stays open for one further message and a long conversation does not slowly - * mount every area's tools — the reason areas exist. + * Only what this turn opened is stored with its reply ([openedThisTurn]); the visit's set is read back + * from those ([areasOpenedThisVisit]), so nothing inherited is stored twice. */ internal class OpenAreas( carriedOver: Set = emptySet(), @@ -27,6 +29,19 @@ internal class OpenAreas( } } +/** + * The areas any reply of the current visit opened, for the transcript read oldest first. + * + * A visit begins at the last greeting, the same boundary the manager sees, so a new visit starts with + * nothing mounted. Within one, an area stays open: an area is only ever opened because the manager asked + * about something in it, so what accumulates is what they have actually been working on. + */ +internal fun List.areasOpenedThisVisit(): Set = + drop(indexOfLast { it.opening }.coerceAtLeast(0)) + .filter { it.role == BuddyMessageRole.ASSISTANT } + .flatMap { it.openedAreas.toTeamAreas() } + .toSet() + /** The stored form of the areas a reply opened: their names, comma-separated and sorted; null when none. */ internal fun Set.encoded(): String? = takeIf { it.isNotEmpty() }?.sortedBy { it.name }?.joinToString(",") { it.name } diff --git a/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCall.kt b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCall.kt new file mode 100644 index 00000000..9e2a7650 --- /dev/null +++ b/src/main/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCall.kt @@ -0,0 +1,20 @@ +package com.sprintstart.sprintstartbackend.onboarding.service + +private val NAMED_CALL = Regex(""""name"\s*:\s*"[\w.-]+"""") +private val CALL_ARGUMENTS = Regex(""""(?:parameters|arguments)"\s*:""") + +/** + * Whether a model's final answer is a tool call written out as text rather than made. + * + * A small model sometimes answers `{"name":"find_member","parameters":{...}}` as its reply. Nothing ran, + * and the manager reads a broken request as if it were an answer. The shape is checked, not the tool + * name: a call to a tool that is not mounted is the same failure, and the catalogue changes. + */ +internal fun String.writesOutAToolCall(): Boolean = + NAMED_CALL.containsMatchIn(this) && CALL_ARGUMENTS.containsMatchIn(this) + +/** Told to the model in place of showing the manager a call that never ran. */ +internal const val TOOL_CALL_WRITTEN_OUT = + "That reply was a tool call written out as text, so nothing ran and the manager has not seen an answer. " + + "If you need a tool, call it properly (open its area with open_area first if it is not available). " + + "Otherwise answer the manager in plain words." diff --git a/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql b/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql index 60dcd77c..42cfc94b 100644 --- a/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql +++ b/src/main/resources/db/migration/V20__add_buddy_team_message_opened_areas.sql @@ -1,4 +1,4 @@ --- The team areas a buddy reply opened, so the manager's next message still has the tools that area mounts. +-- The team areas a buddy reply opened, so the manager's later messages in the visit still have the tools that area mounts. -- Nullable on purpose: existing messages opened nothing that matters, and Hibernate (ddl-auto: update) can add -- a nullable column to a populated table, which it cannot do for NOT NULL ones. Idempotent. ALTER TABLE buddy_team_messages diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt index 3a840ddd..0ba605e5 100644 --- a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamServiceTest.kt @@ -285,7 +285,7 @@ class BuddyTeamServiceTest { } @Test - fun `opening the area again keeps it open for one more message`() = runTest { + fun `an area opened again is stored again with the reply that opened it`() = runTest { asTranscript(reply(opened = "KNOWLEDGE")) val saved = savedReplies() mountedOnEachHop() @@ -312,27 +312,98 @@ class BuddyTeamServiceTest { } @Test - fun `nothing is carried over from anything but the reply just before`() = runTest { - val mountedSets = mountedOnEachHop() - coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Ok.") - - // An older reply opened an area, but a later one did not: the area has closed. + fun `an area stays mounted through a discussion, however many messages come before the approval`() = runTest { asTranscript( + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "can you answer Ada?"), reply(opened = "KNOWLEDGE"), - BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "thanks"), - reply(content = "You are welcome."), + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "make it shorter"), + reply(content = "Shorter draft."), + BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "friendlier please"), + reply(content = "Friendlier draft."), ) - service.sendMessageForMe(authId, projectId, "and now?").toList() + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Done.") - // The last message is a greeting that opens a visit. - asTranscript(reply(content = "Hi again.", opening = true)) + service.sendMessageForMe(authId, projectId, "You can send it").toList() + + assertThat(mountedSets).containsExactly(setOf(TeamArea.KNOWLEDGE)) + } + + @Test + fun `every area the visit opened stays mounted`() = runTest { + asTranscript(reply(opened = "KNOWLEDGE"), reply(opened = "TEAM,ARRIVAL")) + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Done.") + + service.sendMessageForMe(authId, projectId, "go ahead").toList() + + assertThat(mountedSets).containsExactly(setOf(TeamArea.KNOWLEDGE, TeamArea.TEAM, TeamArea.ARRIVAL)) + } + + @Test + fun `a new visit starts with nothing mounted`() = runTest { + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Ok.") + + // What an earlier visit opened is closed by the greeting that begins the next one. + asTranscript(reply(opened = "KNOWLEDGE"), reply(content = "Hi again.", opening = true)) service.sendMessageForMe(authId, projectId, "hello").toList() - // The last message is the manager's own, so there is no reply to inherit from. + // No reply yet in this visit, only the manager's own message. asTranscript(BuddyTeamMessage(session = session, role = BuddyMessageRole.USER, content = "hello?")) service.sendMessageForMe(authId, projectId, "anyone there?").toList() - assertThat(mountedSets).containsExactly(emptySet(), emptySet(), emptySet()) + assertThat(mountedSets).containsExactly(emptySet(), emptySet()) + } + + @Test + fun `an area opened in this visit survives a greeting that came before it`() = runTest { + asTranscript(reply(opened = "TEAM"), reply(content = "Hi again.", opening = true), reply(opened = "KNOWLEDGE")) + val mountedSets = mountedOnEachHop() + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply("Done.") + + service.sendMessageForMe(authId, projectId, "You can send it").toList() + + assertThat(mountedSets).containsExactly(setOf(TeamArea.KNOWLEDGE)) + } + + /** + * Seen live: the model answered with `{"name":"find_member","parameters":{...}}` as its reply. Nothing + * ran, and the manager was shown a broken request as if it were an answer. + */ + @Test + fun `a tool call written out as the reply is sent back rather than shown`() = runTest { + val requests = mutableListOf() + val written = """I'll look them up. {"name":"find_member","parameters":{"query":"Ada"}}""" + coEvery { onboardingAiClient.buddyAgentTurn(capture(requests)) } returnsMany listOf( + BuddyAgentResponse( + final = true, + text = written, + messages = listOf(BuddyAgentMessageDto(role = "assistant", content = written)), + ), + finalReply("Ada is on the project."), + ) + + val events = service.sendMessageForMe(authId, projectId, "how is Ada?").toList() + + assertThat(requests).hasSize(2) + assertThat(requests[1].messages.map { it.role }.takeLast(2)).containsExactly("assistant", "user") + assertThat(requests[1].messages.last().content).isEqualTo(TOOL_CALL_WRITTEN_OUT) + assertThat(events.mapNotNull { it.content }.joinToString("")).isEqualTo("Ada is on the project.") + } + + @Test + fun `a model that keeps writing the call out ends in the fallback reply, never the raw call`() = runTest { + val saved = savedReplies() + val written = """{"name":"find_member","parameters":{"query":"Ada"}}""" + coEvery { onboardingAiClient.buddyAgentTurn(any()) } returns finalReply(written) + + val events = service.sendMessageForMe(authId, projectId, "how is Ada?").toList() + + assertThat(events.mapNotNull { it.content }.joinToString("")).isEqualTo(BuddyService.FALLBACK_REPLY) + assertThat(saved.single { it.role == BuddyMessageRole.ASSISTANT }.content) + .isEqualTo(BuddyService.FALLBACK_REPLY) + coVerify(exactly = BuddyService.MAX_AGENT_STEPS) { onboardingAiClient.buddyAgentTurn(any()) } } @Test diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt index b3f86602..8d338b5a 100644 --- a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/BuddyTeamToolsTest.kt @@ -396,13 +396,13 @@ class BuddyTeamToolsTest { } @Test - fun `open_area's definition says an area stays open for one more message, and forbids inventing a confirmation`() { + fun `open_area's definition says an area stays open for the visit, and forbids inventing a confirmation`() { val tools = tools(actions = listOf(action("answer_escalation", TeamArea.KNOWLEDGE))) val description = tools.toolSpecs(emptySet()).single { it.name == BuddyTeamTools.OPEN_AREA }.description - assertThat(description).contains("stays open for the manager's next message too") - assertThat(description).contains("open the area again first") + assertThat(description).contains("stays open for the rest of this visit") + assertThat(description).contains("open its area first") assertThat(description).contains("Never say something has been offered for confirmation unless a tool") } } diff --git a/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCallTest.kt b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCallTest.kt new file mode 100644 index 00000000..0748db41 --- /dev/null +++ b/src/test/kotlin/com/sprintstart/sprintstartbackend/onboarding/service/WrittenOutToolCallTest.kt @@ -0,0 +1,23 @@ +package com.sprintstart.sprintstartbackend.onboarding.service + +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test + +class WrittenOutToolCallTest { + @Test + fun `recognises a call written out as text, valid JSON or not`() { + assertThat("""{"name":"find_member","parameters":{"query":"Ada"}}""".writesOutAToolCall()).isTrue() + assertThat("""{"name": "open_area", "arguments": {"area": "knowledge"}}""".writesOutAToolCall()).isTrue() + // Invalid JSON, as the model wrote it. + val broken = """Let me check. {"name":"find_member","parameters":{"query":""[the name]""}}""" + assertThat(broken.writesOutAToolCall()).isTrue() + } + + @Test + fun `an ordinary answer is not a call, even one that mentions a tool or a name`() { + assertThat("Ada is waiting on a review; I looked her up with find_member.".writesOutAToolCall()).isFalse() + assertThat("""Her name is "Ada" and that is all.""".writesOutAToolCall()).isFalse() + assertThat("""{"name":"Ada"}""".writesOutAToolCall()).isFalse() + assertThat("".writesOutAToolCall()).isFalse() + } +}