fix(memory): reject reused branch IDs when creating a branch

[hsusul]

Summary

AdvancedSQLiteSession.create_branch_from_turn() never checked whether the target branch ID was already in use. _copy_messages_to_new_branch() inserted message_structure rows tagged with that ID unconditionally, so creating a branch whose ID already existed appended the copied history to that branch instead of creating a new one. The call still returned the ID and switched to it, so the caller got no signal.

Two ways to hit it:

1. The generated default ID can collide. With branch_name omitted the ID is f"branch_from_turn_{turn_number}_{int(time.time())}". That is one-second resolution, so branching twice from the same turn within the same second — the documented await session.create_branch_from_turn(2) form — reuses the ID and silently duplicates the copied history. No user error is involved.
2. An explicit existing name was accepted, including "main", which duplicated the base branch's own history into itself.

This resolves the target branch ID inside the locked connection that performs the copy: an explicit ID that is already in use raises ValueError, and a generated ID is disambiguated with a numeric suffix.

Affected component: src/agents/extensions/memory/advanced_sqlite_session.py (AdvancedSQLiteSession conversation branching). create_branch_from_content() inherits the fix since it delegates.

Minimal reproduction (no API key, no model call, no network):

import asyncio
import time
from unittest.mock import patch

from agents.extensions.memory import AdvancedSQLiteSession

ITEMS = [
    {"role": "user", "content": "turn one"},
    {"role": "assistant", "content": "reply one"},
    {"role": "user", "content": "turn two"},
    {"role": "assistant", "content": "reply two"},
    {"role": "user", "content": "turn three"},
    {"role": "assistant", "content": "reply three"},
]


async def main() -> None:
    session = AdvancedSQLiteSession(session_id="collide", create_tables=True)
    await session.add_items(ITEMS)
    with patch.object(time, "time", lambda: 1_700_000_000.0):
        first = await session.create_branch_from_turn(3)
        await session.switch_to_branch("main")
        second = await session.create_branch_from_turn(3)
    print(first == second, len(await session.get_items()))
    session.close()


asyncio.run(main())

Current behavior: prints True 8. Both calls return branch_from_turn_3_1700000000, the branch holds the first two turns twice, and list_branches() reports one branch where the caller created two. Separately, create_branch_from_turn(2, "main") grows main from 6 items to 8.

Corrected behavior: prints False 4. The two calls produce distinct IDs and each branch holds exactly one copy of the pre-branch history. create_branch_from_turn(2, "main") now raises ValueError and leaves main untouched.

Root cause

_copy_messages_to_new_branch() selected the rows to copy and inserted them under new_branch_id with no existence check on that ID, and create_branch_from_turn() derived its default ID from a one-second timestamp without consulting existing branches.

Implementation

• _branch_id_exists() — a small helper that asks whether any message_structure row already uses a branch ID, matching how switch_to_branch(), delete_branch(), and list_branches() already define branch existence.
• _generate_branch_id() — appends a numeric suffix to the timestamp-based base name until it is unused.
• _copy_messages_to_new_branch() now takes str | None and returns the resolved branch ID. Both the existence check and the ID generation run against the cursor that performs the inserts, inside the same _locked_connection(), so the check cannot be separated from the write by a concurrent branch creation. Doing this in a separate asyncio.to_thread() step would release the lock in between.
• create_branch_from_turn() drops its local ID generation and uses the returned ID for the branch pointer, the debug log, and its return value.

Why this is minimal: no public API signature changes, no schema change, and the copy semantics (which rows are copied, sequence numbering, shared message rows) are untouched. The rejection uses ValueError, which the method already documented for invalid branch requests and which delete_branch() and switch_to_branch() already raise for branch-state errors. The ValueError is raised before any DML, so a rejected call performs no write and leaves the current-branch pointer alone.

Test plan

Six regression tests in tests/extensions/memory/test_advanced_sqlite_session.py. All five that assert the corrected behavior fail on upstream/main at c546ca1 and pass with the fix; the sixth is a boundary guard that passes both before and after.

Test	Covers
test_create_branch_rejects_an_existing_branch_id	Explicit reused ID raises, both branches and the current pointer are unchanged
test_create_branch_rejects_the_main_branch_id	Branching into main raises instead of duplicating it into itself
test_generated_branch_ids_stay_unique_within_the_same_second	Frozen clock, two branches from the same turn get distinct IDs and one copy of history each
test_generated_branch_ids_stay_unique_across_many_calls	Frozen clock, three consecutive generated IDs stay distinct and correct
test_create_branch_from_content_rejects_an_existing_branch_id	create_branch_from_content() inherits the contract
test_create_branch_from_the_first_turn_still_succeeds	Branching from turn 1 copies nothing and remains allowed (passes before and after)

The clock is frozen with monkeypatch.setattr(time, "time", ...), so the collision is deterministic rather than timing-dependent. No API key, model call, network access, or sleep is involved; the sessions are in-memory SQLite.

Before the fix:

$ uv run pytest tests/extensions/memory/test_advanced_sqlite_session.py -q -k "branch_rejects or generated_branch or from_content_rejects or first_turn_still"
5 failed, 1 passed, 67 deselected in 0.41s

After the fix:

$ uv run pytest tests/extensions/memory/test_advanced_sqlite_session.py -q
73 passed in 0.32s

$ uv run pytest tests/extensions/memory/ -q
288 passed, 4 skipped in 8.59s

The focused module was run 5x with -p no:randomly and the memory suite 3x under the default random ordering; results were identical each time.

Full verification stack via .agents/skills/code-change-verification/scripts/run.sh — all commands passed:

make format     -> 847 files left unchanged; ruff check --fix: All checks passed!
make lint       -> passed in 0s
make tests      -> passed in 87s
make typecheck  -> passed in 1557s
code-change-verification: all commands passed.

The docs line was added after that run, so uv run ruff format --check . (847 files already formatted), uv run ruff check . (All checks passed), uv run pytest tests/test_doc_parsing.py tests/extensions/memory/test_advanced_sqlite_session.py -q (77 passed), and make build-docs (built successfully, no new warnings) were re-run afterwards. git diff --check is clean.

Compatibility

Callers that previously reused a branch ID now get a ValueError instead of a silently corrupted branch. That path never produced a usable new branch, so no working behavior is lost. Generated IDs keep their existing branch_from_turn_{turn}_{timestamp} shape and only gain a _2, _3, ... suffix when that exact ID is already taken. The stored schema, the copy semantics, and every public signature are unchanged.

Non-goals

• No change to which rows are copied or to the exclusive-turn semantics of from_turn_number.
• No validation of empty or whitespace branch names — unrelated to the demonstrated defect.
• No change to switch_to_branch(), delete_branch(), or clear_session().

Limitations

• The ValueError covers a branch that already holds messages. Branching from turn 1 copies no rows, so such a branch leaves no message_structure row and is not "existing" by the definition the rest of the class already uses; that behavior is unchanged and is covered by test_create_branch_from_the_first_turn_still_succeeds.
• The check is atomic per connection lock, which is the same scope every other write in this class relies on. Cross-process access to the same SQLite file is outside that scope, as it already was.
• Only docs/sessions/advanced_sqlite_session.md was updated; the generated docs/ja, docs/ko, and docs/zh pages were intentionally left alone.

Issue number

Closes #4150

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all verification steps pass
• If using Codex, I've run /review before submitting this PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c48e2f26c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Track branch identity independently of message rows

When a branch is created from turn 1, it copies no messages, so this query records no evidence that its ID exists and a subsequent explicit or same-second generated creation can reuse that ID. The same check is also racy when separate processes share a file-backed database: the lock is explicitly process-local in src/agents/memory/sqlite_session.py, and without a write transaction or unique database key both processes can observe the ID as absent and then merge copied histories under it. Persist branch identity under a unique (session_id, branch_id) constraint and allocate it transactionally so the documented uniqueness guarantee applies to empty branches and concurrent writers.

AGENTS.md reference: AGENTS.md:L134-L134

Useful? React with 👍 / 👎.

[seratch]

Please resolve all the review feedback from codex first.

[seratch]

Thanks again for working on this issue here. Your contribution will be included in #4186