Skip to content

Support short session IDs in transcript recall - #122

Merged
Diego Colombo (colombod) merged 9 commits into
mainfrom
fix/transcript-short-session-ids
Sep 19, 2026
Merged

Diego Colombo (colombod) merged 9 commits into
mainfrom
fix/transcript-short-session-ids

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Summary

Make /transcript <short-id> retrieve the named session immediately instead of interpreting the ID as an instruction about the current session. Add reusable, host-independent resolve_session_id(reference, candidates) lookup: exact IDs take precedence, prefixes require at least eight characters and one unique match, and ambiguity/not-found are explicit errors. The transcript tool returns canonical IDs for pagination; the skill does not ask redundant permission for an already-requested read.

Two small readiness repairs are included in separate commits: refresh the query module's stale shared-library lock to already-published 19da724, and make scripts/validate-full.sh invoke its own prepared CLI with a public Core wheel rather than compiling Core or accidentally using the ambient CLI interpreter.

Scope / guardrails

  • Read-side only. Write-side hook fan-out (fanout.py, _DestinationDispatcher, logging handlers), graph-server APIs, authentication, and session data are untouched.
  • Local lookup scans directory names/metadata existence, not transcript contents. Duplicate capture locations remain ambiguous, including the current workspace. Custom embedding-host resolvers retain control of their own storage scope.
  • No new daemon, index, cache, graph query, or analyst delegation. Other callers can reuse the library; this PR integrates it into transcript recall only.
  • Runtime and transcript fixtures are synthetic. Raw DTU logs, credentials, host paths, and real conversation data are not committed.

Verification

  • Module tests pass — query: 195, both standalone with its refreshed frozen lock and in the DTU. Transcript: 29, using the installed library at its declared pin. Other module suites: hook 700, lockdown 28, recover 29, upload 554, server-data-ops 65.
  • Top-level tests pass — 881 using the CI command uv run --frozen pytest tests/ -q --tb=short --ignore=tests/dtu; includes six validation-launcher regression tests.
  • ruff check + ruff format --check clean — 179 files checked for formatting.
  • pyright clean — 0 errors, root and transcript module.
  • Full bundle validation — the exact repaired scripts/validate-full.sh completed at 10f348a, with validation_mode: full, actual wheel build PASS, 12/12 bundles loaded, and all 8 modules' dependency references resolved. The recipe's source-reviewed final assessment is PASS WITH WARNINGS. Its raw mechanical verdict remains FAIL because of the pre-documented mode-name false positive; this was not hidden or suppressed.

The full report independently checked the flagged source: the mode-name match is a directory/bundle reference, not an invocation (AGENTS.md:5-28), and the README's unflagged/flagged install pair intentionally documents app-layered and standalone installation. The remaining warnings are the pre-existing long server-data-ops description and a by-design navigation-tool count. Neither is changed by this PR or treated as a new blocking defect.

The upload suite's runtime parity checks passed on all six fresh DTU sessions: zero workspace differences, zero data-reassembly defects. A separate host-corpus run exposed four historical workspace-path mismatches; no source or historical data was changed to hide those. The isolated DTU run exercises the actual current runtime.

Real evidence on seams (not mock-only)

  • Actual CLI slash parsing, skill loading, installed tool, and native capture lookup exercised in a DTU with the candidate served by Gitea. Commands were typed at a fresh interactive prompt, not passed as initial amplifier run prompts.
Model Unique prefix Ambiguous prefix Missing prefix
Anthropic claude-sonnet-5 PASS PASS PASS
OpenAI gpt-5.6-terra PASS PASS PASS

Each case used one slash command, with no confirmation reply or retry. Both providers called session_transcript with the supplied short ID. Unique-prefix replay returned the intended full ID and both seeded user/assistant markers. Ambiguity returned two candidates without replay; missing IDs returned no session matches. All six CLI sessions exited cleanly.

Behavioral tests used bb1cc8c; the only subsequent change, 10f348a, repairs the validation launcher's Foundation dependency override and its regression test. Transcript tool/library/skill bytes are unchanged. Earlier Haiku/GPT-5-mini runs also passed 6/6 after catching and fixing the redundant-confirmation behavior.

Docs & diagrams

  • bundle.dot / bundle.png — unchanged: no bundle/behavior structure changed.
  • README and transcript skill describe short IDs, ambiguity, and canonical-ID pagination.
  • Convention files updated for the corrected full-validation launcher and interpreter requirement.

Notes / follow-ups

  • Transcript module dependency is pinned to ancestor commit 2818e0519f1c59c71d6a5b53eee50592853f8d7e, which introduces the reusable library. Preserve that commit when landing, or update/revalidate the pin if history is rewritten.
  • No changes to the separate second-opinion PR or retrospective-skill work. No merge or host installation is part of this PR.

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

@colombod Diego Colombo (colombod) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by pulling the branch down and running everything locally. The server-risk question is clean — I verified there is none. Blocking on one read-side defect that returns the wrong session's transcript with success=True, plus a landing constraint on the dependency pin.

What I verified, and what passed

  • 881 top-level tests pass — uv run --frozen pytest tests/ -q --ignore=tests/dtu, reproduced independently.
  • ruff check + ruff format --check clean — 179 files.
  • Wheel builds and ships context_intelligence/session_ids.py — confirmed by inspecting the built artifact, so there is no "module imports a file the wheel omits" failure.
  • No server-facing surface is touched. The diff of the shared library between main and this branch is __init__.py (+8, additive exports only) and the new session_ids.py (60 lines). No changes under fanout, upload, hook handlers, auth, client.py, destinations, or graph APIs. resolve_session_id takes an iterable of strings — it has no I/O and cannot reach a server. __init__.py adds three names and removes none; no import cycle.
  • The query-module lock bump is a catch-up, not a jump. That module's requirement is @main, so unlocked installs already resolved to 19da724; refreshing the lock makes --frozen agree with what everyone already got. Worth noting it goes stale again the moment this merges.

The read-side/server-isolation claim in the description holds up.


Blocking — an exact, full session ID can return a different session's transcript

_resolve_locator lost its direct-path early return:

-   if locator.metadata_path.is_file() and locator.events_path.is_file():
-       return locator

Every lookup now goes through prefix scanning — including an exact full ID, and including /transcript with no arguments. Child session IDs in this ecosystem are {parent_id}_{agent}, so every parent ID is a strict prefix of every one of its children. Real directory names from a live capture store:

0000000000000000-2a962073a0a64673_foundation-explorer
0000000000000000-730e5665ff8b422c_foundation-git-ops

A probe against that ID shape:

[B] session_ids=[<full parent id>]  -> success=True
    returned_id = <parent-id>_foundation:explorer
    text        = === SESSION <parent-id>_foundation:explorer ===  CHILD CONTENT

[C] no arguments (current session)  -> same substitution

[D] exact valid full parent id, 2+ children -> ambiguous_session (hard failure)

The trigger is the parent's metadata.json being absent while a child's is present — reachable via lazy metadata init (created on the first event), server-data-ops deletion, or ingestion filtering. The result is a successful call returning another session's conversation, on the most common invocation path in the feature.

Requested change: restore the exact-path early return ahead of the prefix scan, and add the parent-with-missing-metadata-plus-one-child case to tests/test_session_transcript_tool.py. The existing suite does not cover it — test_duplicate_capture_locations_are_ambiguous_even_with_same_full_id covers duplicate roots, not prefix shadowing by a child.

Blocking — the 8-character minimum does not fit the real ID space

Measured against a live capture store:

total session dirs across all projects: 5893
beginning '00000000':                   4427  (75%)
'/transcript 00000000' -> ambiguous_session, matches 4427, shows 5 candidates

The 8-character floor is calibrated for UUID randomness. Sub-agent IDs begin with a zero-padded 17-character block, so a prefix does not discriminate until character 18. For roughly three-quarters of captured sessions the headline feature resolves to an ambiguity error, and the "use a longer prefix" hint is unactionable because the distinguishing bytes sit past the part a user would ever type.

To be clear about severity: this fails safe. resolve_session_id refuses to guess, which is the right design and is why this is a quality issue rather than a correctness one. But tests/test_session_ids.py uses exactly this ID shape:

child = "0000000000000000-abcdef1234567890_self"
assert resolve_session_id("0000000000000000-abcdef12", [child]) == child

— with a single candidate, which is precisely the arrangement that hides the collision.

Requested change: either make the minimum a distinguishing-length rule rather than a flat 8 characters, or document that sub-agent IDs require an 18+ character prefix and make the ambiguity message say so. A test with several same-block-prefixed candidates would have caught this.

Landing constraint — the dependency pin is not reachable from main

git merge-base --is-ancestor 2818e0519f1c59c71d6a5b53eee50592853f8d7e main  ->  NO

The behavior YAMLs install this module from @main, so post-merge installs will fetch 2818e05. A squash merge orphans it, and a rebase merge rewrites the SHA outright. GitHub retains refs/pull/122/head so it stays fetchable in practice, but the shipped module would depend on a commit on no branch.

The description already flags this. Making it explicit as a merge-time requirement: merge with a merge commit, or re-point the pin to the post-merge main SHA and re-lock before landing. Please do not squash or rebase-merge this PR as it stands.

Non-blocking

Performance, common path. Resolution moved from a direct stat to a two-level scan of the whole capture store, per session, per call:

direct stat (previous, current session)     0.042 ms
scan, exact full id                        23.7   ms
scan, zero-prefixed id                    339.7   ms     (170 projects, 5893 sessions)

Warm cache; worse on a cold or network filesystem. Restoring the early return above recovers most of this for free.

Skill prompt over-triggers. skills/transcript/SKILL.md says bare tokens "containing a hyphen, underscore, or digit also select a session". That makes /transcript chapter-2 or /transcript summarize-day-3 resolve as a session ID and fail, rather than being read as intent. Worth tightening the heuristic toward the actual ID shapes.

Version. The root package stays 0.1.3 while its public API gains three exports. Consumers pin by git SHA so this is mostly moot, but a bump would make the addition legible.


Summary: the isolation story is sound and the verification in the description largely reproduces. The prefix widening is the problem — loosening a match from == to startswith is only safe when no valid identifier is a prefix of another, and {parent}_{agent} child IDs violate that by construction. Restoring the exact-match fast path addresses the blocking defect and most of the performance regression in one change.

…rong session

Prefix-based session ID lookup could return a different session's transcript
with success=True when the requested ID was a prefix of another session's
capture directory name.

Root cause: spawned sub-agent capture directories are named
`{parent_session_id}_{agent}`, making every session ID a prefix of captures it
spawned. The metadata lookup only treated directories as candidates after
`metadata.json` was written, which happens lazily on the first event. A session
still initializing would drop out of the candidate set, allowing one of its own
sub-agents to win instead. This affected both the no-argument `/transcript` path
(current session) and explicit prefix lookups.

Two guards were added:
- An exactly-named capture directory is now recognized as the requested session
  even before metadata exists (reported as capture_unavailable)
- An ID whose only matches are derived sub-agent captures is reported as
  session_not_found, naming those sub-agents rather than silently replaying one

The existing prefix-scan logic is preserved, maintaining the duplicate-capture-root
ambiguity guarantee.

Fixes: five regression tests added; four were failing before this change.

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ition

The ambiguity error message previously said "use a longer prefix or full ID",
which was unactionable when matches shared a long leading run. Because spawned
sub-agent IDs begin with a zero-padded parent block, a real capture store could
have 4456 sessions matching an 8-character prefix, with the distinguishing
character at position 18.

The message now computes the shared prefix length across all matches and reports
the exact divergence point: "all of them share their first 17 characters, so
supply at least 18 characters or an exact full ID".

Also handles the case where one match is a strict prefix of another: no longer
prefix can separate them, so only an exact full ID can select it.

Unchanged: the 8-character safety floor, ambiguous_session error code, and the
bounded 5-sorted-candidate contract.

Fixes: three regression tests added; all were failing before this change.

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Updated skill documentation to reflect the two preceding fixes.

Skill changes:
- Clarified ID shape guidance: "8+ hex characters, a full UUID, or the zero-padded
  sub-agent form" replace the vague "containing a hyphen, underscore, or digit"
  which incorrectly swallowed intent text like `/transcript chapter-2`
- Specified that ambiguous tokens resolve toward intent, because visible intent
  errors are recoverable while wrong lookups just fail
- Added guidance to relay the divergence position reported by the tool
- Specified that capture_unavailable and derived-sub-agent session_not_found
  errors should be relayed rather than substituted

Skill version: 1.1.0 → 1.2.0

README updates:
- Stale ambiguity-handling paragraph was updated to match current code behavior

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@colombod

Copy link
Copy Markdown
Collaborator

Pushed three commits addressing my review, with Brian's go-ahead. Additive only — no force-push, no rebase, all six original commits (2818e05…10f348a) intact.

Commit What
f22da98 fix(transcript): prevent prefix-based session lookup from returning wrong session
a3688ff fix(session-ids): improve ambiguity error message with divergence position
48e80e7 docs(transcript): clarify session ID shapes and ambiguity handling

1. Blocking defect — exact ID answered with a different session

The prefix scan is kept, so the duplicate-capture-root ambiguity guarantee this PR deliberately built is untouched. Two guards were added to _find_capture_metadata instead of restoring a blanket early return:

  • An exactly-named capture directory is the requested session, even before its metadata.json exists. The logging handler writes that file lazily on the first event, so a session still initialising dropped out of the candidate set and one of its own {id}_{agent} sub-agents won. Now reported as capture_unavailable.
  • An ID whose only matches are derived sub-agent captures is reported as session_not_found naming those sub-agents, instead of silently replaying one.

Five regression tests added. Four were red before:

FAILED test_initialising_capture_never_replaced_by_derived_sub_agent
    assert not True  -> ToolResult(success=True, ... 'session_id': 'abcdef12-123...
FAILED test_current_session_never_replaced_by_its_own_sub_agent        # the no-argument path
FAILED test_initialising_capture_is_not_ambiguous_between_its_sub_agents
    assert 'ambiguous_session' == 'capture_unavailable'
FAILED test_id_with_only_derived_captures_is_reported_not_substituted

Green after: 5 passed, 26 deselected.

The fifth test is a guard in the other direction — a genuine prefix that stops short of the _ derivation mark still resolves normally, so the headline short-ID behaviour is not regressed. It was green before and after.

The original probe that found this, re-run against the fixed code:

[A] exact parent wins: OK
[B] success=False  capture_unavailable: session '4208785c-...' has a capture directory
                   but no readable metadata.json; its capture is incomplete or still initialising
[C] no-arg  success=False  capture_unavailable        (was: returned the sub-agent's transcript)
[D] success=False  capture_unavailable                (was: ambiguous_session for a valid exact ID)

2. Blocking defect — unactionable ambiguity hint

resolve_session_id now computes the shared prefix length across matches and reports where they actually diverge. On the live capture store:

before: '00000000' matches 4456 sessions; use a longer prefix or full ID. Candidates (up to 5): ...
after:  '00000000' matches 4456 sessions; all of them share their first 17 characters,
        so supply at least 18 characters or an exact full ID. Candidates (up to 5): ...

after:  '0000000000000000-2' matches 287 sessions; all of them share their first 18 characters,
        so supply at least 19 characters or an exact full ID. ...

18 is the true answer for that ID shape, and the caller can now act on it.

It also fixes a case the old message got quietly wrong: when one match is a strict prefix of another, no longer prefix can separate them, so it now says only an exact full ID can select it rather than sending the caller after a prefix that cannot exist.

Deliberately unchanged: the 8-character safety floor, the ambiguous_session error code, and the bounded 5-sorted-candidate contract. No caller and no error-type mapping is affected. The behaviour still fails safe — this only makes the failure useful.

Three regression tests added, all red before. One uses several same-block-prefixed sub-agent IDs — the arrangement the existing single-candidate test could not catch.

3. Non-blocking — skill prompt, and stale README

The skill's "bare token containing a hyphen, underscore, or digit" rule swallowed ordinary intent. It now names the real ID shapes (8+ hex, full UUID, or the zero-padded sub-agent form) and states that an ambiguous token resolves toward intent — a wrong intent is visible and recoverable, a wrong lookup just fails. Version 1.1.0 → 1.2.0.

The README paragraph on ambiguity handling went stale against the code changes above, so it was updated in the same commit.


Verification

uv run --frozen pytest tests/ -q --tb=short --ignore=tests/dtu     884 passed   (was 881; +3 new)
transcript module: uv run --frozen pytest tests/ -q                 34 passed   (was 29; +5 new)
ruff check .                                                        All checks passed!
ruff format --check .                                               179 files already formatted
pyright                                                             0 errors, 0 warnings, 0 informations
uv build --wheel  ->  context_intelligence/session_ids.py shipped:  True

No probe or scratch file left in the tree; the six changed files are the whole delta.

Correction to my own review

My review quoted the prefix scan at 339.7 ms. That was a cold-cache measurement and I should have said so. Measured properly — five runs, median, same warm cache, pre- and post-fix:

                    pre-fix     post-fix
exact full id       22.6 ms      24.0 ms
'00000000'          71.2 ms      77.2 ms

So the scan cost is ~71 ms warm, not 340, and my guards add ~1–6 ms. More to the point: I was wrong that fixing the correctness defect would recover the performance. The scan is load-bearing for the cross-root ambiguity guarantee — you cannot detect a duplicate capture root without looking at every root. The cost is inherent to the design, not a bug, and I'm withdrawing that part of the review. Worth a follow-up issue if the store keeps growing (171 projects / ~5.9k sessions here), not a change to this PR.

Still outstanding — not fixed here

  • The dependency pin. 2818e05 is still not reachable from main, and cannot be corrected until this lands. The merge-commit requirement stands: please do not squash or rebase-merge. Alternatively, re-point the pin to the post-merge main SHA and re-lock.
  • Root package version stays 0.1.3 despite the public API gaining exports. Bumping it ripples into two lockfiles that pin by git SHA anyway, so I left it; flagging rather than deciding.

Happy to re-review, or to take any of these in a different direction if you'd rather.

@colombod

Copy link
Copy Markdown
Collaborator

Brian Krabach (@bkrabach) — fixes are pushed and CI is green. Handing this back to you for approval and merge, with one correction to the description.

The description now overstates its evidence

The verification section says:

Behavioral tests used bb1cc8c; the only subsequent change, 10f348a, repairs the validation launcher's Foundation dependency override and its regression test. Transcript tool/library/skill bytes are unchanged.

That last sentence is no longer true — my three commits changed exactly those bytes:

  • session_transcript_tool.py (resolution guards)
  • context_intelligence/session_ids.py (ambiguity message)
  • skills/transcript/SKILL.md (ID-shape wording, 1.1.0 → 1.2.0)

So the cross-provider DTU matrix (Anthropic + OpenAI, six sessions) and the full bundle validation at 10f348a no longer cover the code on this branch. Worth editing that line before merge so the next reader isn't relying on evidence that has moved out from under the code.

What does still cover it: unit tests, including seven new regression tests — four of which reproduced the wrong-session defect before the fix — and all 12 CI checks green on head 48e80e7:

Lint                                        pass
Tests — root (3.11 / 3.12 / 3.13)           pass
Tests — tool-context-intelligence-transcript pass
Tests — hook-context-intelligence            pass
Tests — tool-context-intelligence-query      pass
Tests — tool-context-intelligence-recover    pass
Tests — tool-context-intelligence-upload     pass
Tests — tool-server-data-ops                 pass
Tests — hook-server-data-ops-lockdown        pass
license/cla                                  pass

What is not re-run: the DTU behavioural matrix and full bundle validation. Your call whether either is worth repeating — the happy path is unchanged by design (a regression test pins that a genuine prefix still resolves normally; the new code adds guards on failure paths only).

Why I'm not approving this myself

I requested the changes and then wrote the fixes. Approving now would mean the only review signal on this PR is someone signing off on their own patch, which is the thing review is for. The three commits are f22da98, a3688ff, 48e80e7 — they want your eyes, or another reviewer's, not mine a second time.

My CHANGES_REQUESTED is left standing deliberately so it doesn't merge by accident. Dismiss it whenever you're satisfied.

One trap at merge time

main is not branch-protected and squash and rebase are both enabled, so nothing mechanically stops a squash — and a squash orphans 2818e05, which tool-context-intelligence-transcript pins as its dependency.

Please merge with a merge commit. Or, if you'd rather squash, re-point the pin to the post-merge main SHA and re-lock first.

Happy to do either, or to revisit any of the three fixes if you'd take them a different way.

@colombod
Diego Colombo (colombod) merged commit 98426d9 into main Sep 19, 2026
12 checks passed
@colombod
Diego Colombo (colombod) deleted the fix/transcript-short-session-ids branch September 19, 2026 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants