Skip to content

fix(engine): preserve prompt cache prefix with skill hook context - #205

Open
Putluru Sandeep (sunny-kavixo) wants to merge 3 commits into
microsoft:mainfrom
sunny-kavixo:fix/579-skill-context-cache
Open

Putluru Sandeep (sunny-kavixo) wants to merge 3 commits into
microsoft:mainfrom
sunny-kavixo:fix/579-skill-context-cache

Conversation

@sunny-kavixo

@sunny-kavixo Putluru Sandeep (sunny-kavixo) commented Oct 13, 2026 •

Copy link
Copy Markdown

Improves Amplifier Agent’s within-turn prompt-prefix stability for request-only hook context.

Skill-hook context was appended as one aggregate to each provider request. New conversation messages moved that aggregate, breaking the growing prompt prefix after compaction. The engine now anchors request-only context batches where they first become active, appends new batches after completed tool results, and resets placement after compaction, history replacement/clearing, or a turn boundary. Context and compaction notices stay out of canonical history.

Prefix stability applies within a turn between compactions. Hook context deliberately expires when the turn succeeds, fails, or is cancelled.

Validation on a7757a0c26676520cd08c205eedccc8d2796ddc9, merged with main ec691a2feed5a628b2c54ad5b370defbb8a85f13:

  • Three focused regression cases fail on unchanged main and pass with the fix.
  • Added coverage for repeated compaction, history replacement and clear, expired batches, request copy isolation, parallel tool results, and real PreToolUse/PostToolUse hooks across success, failure and cancellation.
  • Independent DTU engine/Python/HTTP tests: 967 passed; lint, formatting and type checks passed.
  • Hosted Linux x86_64 checks: 1,119 Python tests and 85 TypeScript tests passed, along with prek run --all-files, scripts/check.py --runtime, and source/wheel builds using the pinned toolchain.

These tests verify request construction and lifecycle behavior. They do not measure live-provider cache-hit rates, billing or latency.

The hosted technical checks and license/cla check pass. Merge remains a decision for the Amplifier Agent maintainers.

@bkrabach Brian Krabach (bkrabach) left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Initial source review of f0cbfeb: stable, request-only hook-context batches within an Amplifier Agent turn avoid rebuilding the cached prompt prefix on every iteration. Placement belongs in the engine/context owner; additions must respect unanswered tool calls. Stability is within a turn, not across deliberate turn-boundary resets.

The review requested coverage for repeated compaction, set_messages/clear, and successful, failed, and cancelled execute cleanup, plus resolution of the base conflict. Those requests were addressed in a7757a0; see the subsequent technical approval for independently run validation. These checks guard against stale anchors and cross-turn context leakage for Amplifier Agent consumers.

@bkrabach Brian Krabach (bkrabach) left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Technical review complete on a7757a0. I resolved the main conflict and added the reset-path and turn-lifecycle coverage requested in the previous review, retaining the original contribution and main's current reasoning behavior.

The independent DTU run passed 967 engine/Python/HTTP tests. The final hosted run passed prek, the runtime build, 1,119 Python tests, 85 TypeScript tests, and package builds: https://github.com/microsoft/amplifier-agent/actions/runs/38381430373.

The request-only placement and turn scope match the intended engine ownership. No remaining technical change is requested. The contributor’s license/cla check now passes as well.

@sunny-kavixo

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@sunny-kavixo

Copy link
Copy Markdown
Author

Hi Brian Krabach (@bkrabach),

Thank you for your technical review and validation of PR #205.

I completed a fresh local merge-readiness audit against the updated main branch. The implementation and regression tests remain sound, but GitHub currently reports a merge conflict limited to CHANGELOG.md.

I've prepared and locally validated a minimal resolution without modifying the production implementation.

Before updating the contributor branch, could you please confirm whether you would prefer me to push the conflict resolution or have a maintainer resolve the changelog conflict during integration?

I want to preserve the existing review and avoid unnecessary branch updates.

Thank you for your guidance.

@sunny-kavixo

Copy link
Copy Markdown
Author

Hi Brian Krabach (@bkrabach),

I hope you're doing well.

Thank you again for your technical review, approval, and the independent validation of PR #205. I really appreciate your time and guidance.

I wanted to follow up on the remaining steps toward merging this contribution. I noticed the PR is still open and GitHub reports a conflict in CHANGELOG.md.

Could you please let me know if there are any outstanding technical concerns, additional changes, or maintainer requirements I should address?

I've already prepared a minimal local resolution for the changelog conflict, and I'm happy to push it if you'd prefer me to handle that on the contributor branch. Alternatively, I can leave the resolution to the maintainers during integration.

I'm keen to make sure the contribution meets the project's standards and can move forward without creating unnecessary work for the team.

Thank you again for your support and consideration.

Best regards,
Putluru Sandeep (Putluru Sandeep (@sunny-kavixo))

@sunny-kavixo

Copy link
Copy Markdown
Author

Rebased on the latest main and resolved the CHANGELOG.md conflict. No production code changes were made. I verified the resulting diff and merge readiness.

Thank you for the technical review and approval. Ready for merge whenever convenient.

Signed-off-by: Sunny <sandeepkisan1432@gmail.com>
@sunny-kavixo

Copy link
Copy Markdown
Author

Updated the contributor branch with merge commit 1bba1ffcb5fa8a9a868c6427977d6905600e81a5, resolving the current CHANGELOG.md conflict against main at 08853e3f3814af4501acc0231d26bb1d6e516a84. The PR diff remains limited to the original five files.

Local validation after the merge: focused regression tests 63 passed; prek run --all-files passed; Ruff and formatting checks passed. The full runtime check passed lint, format, type, and TypeScript checks, then stopped because the local environment lacks PyInstaller for the runtime build. GitHub Check run 40207472964 is awaiting maintainer approval, and the prior technical approval was dismissed by the new commit. Please re-review when convenient.

This branch has not been deployed

No deployments
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.

2 participants