Support short session IDs in transcript recall - #122
Conversation
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>
Diego Colombo (colombod)
left a comment
There was a problem hiding this comment.
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 --checkclean — 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
mainand this branch is__init__.py(+8, additive exports only) and the newsession_ids.py(60 lines). No changes under fanout, upload, hook handlers, auth,client.py, destinations, or graph APIs.resolve_session_idtakes an iterable of strings — it has no I/O and cannot reach a server.__init__.pyadds 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 to19da724; refreshing the lock makes--frozenagree 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 locatorEvery 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>
|
Pushed three commits addressing my review, with Brian's go-ahead. Additive only — no force-push, no rebase, all six original commits (
1. Blocking defect — exact ID answered with a different sessionThe prefix scan is kept, so the duplicate-capture-root ambiguity guarantee this PR deliberately built is untouched. Two guards were added to
Five regression tests added. Four were red before: Green after: The fifth test is a guard in the other direction — a genuine prefix that stops short of the The original probe that found this, re-run against the fixed code: 2. Blocking defect — unactionable ambiguity hint
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 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 READMEThe 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 The README paragraph on ambiguity handling went stale against the code changes above, so it was updated in the same commit. VerificationNo probe or scratch file left in the tree; the six changed files are the whole delta. Correction to my own reviewMy 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: 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
Happy to re-review, or to take any of these in a different direction if you'd rather. |
|
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 evidenceThe verification section says:
That last sentence is no longer true — my three commits changed exactly those bytes:
So the cross-provider DTU matrix (Anthropic + OpenAI, six sessions) and the full bundle validation at 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 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 myselfI 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 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
Please merge with a merge commit. Or, if you'd rather squash, re-point the pin to the post-merge Happy to do either, or to revisit any of the three fixes if you'd take them a different way. |
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-independentresolve_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 makescripts/validate-full.shinvoke its own prepared CLI with a public Core wheel rather than compiling Core or accidentally using the ambient CLI interpreter.Scope / guardrails
fanout.py,_DestinationDispatcher, logging handlers), graph-server APIs, authentication, and session data are untouched.Verification
uv run --frozen pytest tests/ -q --tb=short --ignore=tests/dtu; includes six validation-launcher regression tests.ruff check+ruff format --checkclean — 179 files checked for formatting.pyrightclean — 0 errors, root and transcript module.scripts/validate-full.shcompleted at10f348a, withvalidation_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)
amplifier runprompts.claude-sonnet-5gpt-5.6-terraEach case used one slash command, with no confirmation reply or retry. Both providers called
session_transcriptwith 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 returnedno 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.Notes / follow-ups
2818e0519f1c59c71d6a5b53eee50592853f8d7e, which introduces the reusable library. Preserve that commit when landing, or update/revalidate the pin if history is rewritten.