microsoft / microsoft/amplifier
[amplifier-app-cli] First session save crashes with FileNotFoundError on minimal bundles — get_metadata() called before session dir exists
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 3.1k
- Forks
- 261
- Avg merge
- 3h 28m
- Merged PRs (30d)
- 13
Description
Component: amplifier-app-cli (Issues disabled on that repo; filed here per ecosystem convention)
Summary
The first session save of a brand-new session raises an uncaught FileNotFoundError, because the save path calls SessionStore.get_metadata() — which requires the session directory to already exist — before anything has created that directory. On bundles that happen to include a hook which pre-creates the session directory, the bug is masked. On a minimal bundle without such a hook, session persistence is lost entirely.
Root cause (verified against main @ 5462f1e)
SessionStore.get_metadata() raises when the session dir does not exist — amplifier_app_cli/session_store.py:353-355:
session_dir = self.base_dir / session_id
if not session_dir.exists():
raise FileNotFoundError(f"Session '{session_id}' not found")
The session directory is created only inside SessionStore.save() (session_store.py, session_dir.mkdir(parents=True, exist_ok=True)) — i.e. after the metadata read that precedes it.
Two call sites read metadata before that first save(), neither guarded:
| Call site | Enclosing function | Code |
|---|---|---|
amplifier_app_cli/main.py:3649 |
execute_single() (defined at main.py:3356) |
existing_metadata = store.get_metadata(actual_session_id) or {} |
amplifier_app_cli/main.py:2902 |
_save_session() — nested helper inside interactive_chat() (main.py:2795), invoked at main.py:2957, 3158, 3176 |
existing_metadata = store.get_metadata(actual_session_id) or {} |
Note the or {} is ineffective: get_metadata() raises rather than returning a falsy value.
Correction to the original report: the second call site belongs to
interactive_chat(), not toexecute_single(). Both are on the first-save path, so the impact is as described, but the attribution differs.
Impact
First save of every brand-new session crashes unless some other component has already created the session directory. Effect is total loss of session persistence for minimal bundles — and it is easy to miss, because rich bundles with an ambient logging-style hook incidentally create the directory first and never see it.
Verified vs. inferred
- Verified by direct code inspection at
main(5462f1e): the raise condition inget_metadata(), both unguarded call sites with their enclosing functions and line numbers, thatsave()is what creates the directory, and that notry/exceptwraps either call site. - Reported by the finder, not independently re-run here: the live crash with full traceback on a minimal bundle, and the observation that rich bundles mask it.
- Suggested but NOT rigorously tested: the fix below. The finder reports it was sufficient locally; please treat it as a starting point and verify independently rather than as tested-to-your-standards.
Suggested fix
Guard both call sites:
try:
existing_metadata = store.get_metadata(actual_session_id)
except FileNotFoundError:
existing_metadata = {}
Worth considering instead/in addition, since this is the second-order cause: give SessionStore a non-raising accessor (e.g. get_metadata_or_default()), or have get_metadata() return {} for a not-yet-created session, so future call sites cannot reintroduce this. Whichever is chosen, the "first save of a new session" path deserves a regression test — it is currently only exercised transitively via bundles that mask it.
Possibly related
- #317 — "[app-cli] Write session identity to disk at session start, not at first prompt:complete". If session identity were written at session start, the directory would always exist and this crash would be structurally impossible. These may want to be resolved together.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with amplifier_app_cli/session_store.py, especially get_metadata() and save(), then inspect the two first-save paths in main.py: execute_single() and interactive_chat()/_save_session(). Exercise a brand-new session with a minimal bundle and add a regression test for the first save. Done means the first save no longer raises FileNotFoundError and session persistence works.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100