docs: align contract docs with canonical events and stable source references#40
Conversation
…erences - Remove phantom event names; align to execution:start/end + llm:request/response - Update contract references away from fragile line numbers - Orchestrator contract signature/docs now reflect **kwargs coordinator injection model - Fix related_files paths so scripts/check_related_files.py passes - Docstring-only updates in python/amplifier_core/interfaces.py 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
bkrabach
left a comment
There was a problem hiding this comment.
Thorough Review — PR #40
Thanks for this work, @michaeljabbour — the core intent is solid and addresses real doc-code drift. We verified every claim against the live codebase (LSP, grep, script execution) and the fundamental changes are correct. However, we found several issues that need a follow-up pass before merge.
What we verified ✅
- Phantom events confirmed:
turn:start,turn:end,llm:pre,llm:post— grep across entire codebase returns zero hits outsideCONTRACTS.md. These genuinely do not exist inevents.rs, any Python source, or any test. - Canonical events confirmed:
execution:startatevents.rs:119,execution:endatevents.rs:121,llm:requestatevents.rs:74,llm:responseatevents.rs:76— all present inALL_EVENTSslice and test-verified. - Orchestrator
**kwargsconfirmed: LSP hover oninterfaces.py:37shows**kwargs: Any. The contract doc on main incorrectly showscoordinator: ModuleCoordinator | None = None. Your fix is correct. - All 18 broken paths confirmed:
check_related_files.pycurrently exits 1 with 18 errors on main. Everyrelated_filespath usingamplifier_core/...is broken. Your../../python/amplifier_core/...fixes all resolve to existing files. interfaces.pychange is docstring-only: LSPdocumentSymbolconfirms all 8 class definitions start at line 33+. The diff touches only lines 3–10 inside the module docstring. Zero runtime impact.
Issues to address
1. "41+" should be "41" (factual error)
CONTRACTS.md now says "The full set contains 41+ events." There is a Rust test at events.rs:342 that asserts ALL_EVENTS.len() == 41. The count is exactly 41, not "41+". Please change to "41 events" or "41 canonical events."
2. ai_context/DOCUMENTATION_LINKING.md has 3 broken path templates — MISSED (high priority)
This is the AI-agent guidance document that templates new contract files. It still has amplifier_core/interfaces.py (without python/) at lines 28, 113, and 167. Any AI (or human) following this template will generate files with the same broken related_files paths this PR is fixing. The check_related_files.py script only validates frontmatter, not body text, so it won't catch these. Please fix these 3 references.
3. docs/HOOKS_API.md still has lines: 205-220 (inconsistency)
The PR removes lines: annotations from every contract file but leaves lines: 205-220 in docs/HOOKS_API.md — making it the only surviving line-number anchor after this PR. For consistency with your own stated rationale, please remove this too.
4. docs/specs/PROVIDER_SPECIFICATION.md has a broken path (medium)
Line 20 references amplifier_core/interfaces.py in a table without the python/ prefix. Body text so the script won't catch it, but worth fixing in the same pass.
Non-blocking observations (no changes needed)
ScriptedOrchestratorintesting.py:145is missing**kwargs: It would crash withTypeErrorif called throughAmplifierSession(which always passescoordinator=). Not this PR's problem, but we'll track it separately.- Event table lists
context:compactionbut notcontext:pre_compact/context:post_compact: Developers writing hooks more commonly need the pre/post events. Consider adding them to the "commonly referenced" table in a follow-up. - Hardcoded event strings:
hooks.py:44-51andbindings/python/src/lib.rs:1104-1118define independent string literals rather than importing fromevents.rs/events.py. The new "always use constants" guidance is good aspirational direction, but the codebase has ~20 existing violations. Not blocking for this PR.
Summary
The core work is valuable and we want it merged. The 4 actionable items above are all straightforward fixes in the same vein as the existing changes. Once addressed, this is ready to go. 🙏
Summary
execution:start/execution:end+llm:request/llm:responseevent vocabulary**kwargscoordinator injection modelrelated_filesfrontmatter paths soscripts/check_related_files.pypasses cleanlypython/amplifier_core/interfaces.py(no runtime changes)Files changed
CONTRACTS.mddocs/HOOKS_API.mddocs/contracts/ORCHESTRATOR_CONTRACT.md**kwargsinjection model, event alignmentdocs/contracts/PROVIDER_CONTRACT.mddocs/contracts/TOOL_CONTRACT.mddocs/contracts/HOOK_CONTRACT.mddocs/contracts/CONTEXT_CONTRACT.mdpython/amplifier_core/interfaces.pyVerification evidence
Test plan
scripts/check_related_files.pypasses with 0 errors, 0 warnings🤖 Generated with Amplifier