[dotnet-port-fixes] Align compaction summary marker handling#539
[dotnet-port-fixes] Align compaction summary marker handling#539michelle-clayton-work wants to merge 2 commits into
Conversation
Support raw JSON _is_summary values in compaction message grouping and add parity tests for bool and raw-token cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Aligns the Go compaction message grouping logic with the upstream .NET fix by recognizing _is_summary markers not only as native bool values but also when represented as raw JSON boolean tokens, and adds parity tests to validate the intended grouping behavior.
Changes:
- Extend summary detection to accept
bool,json.RawMessage,*json.RawMessage, and[]byterepresentations. - Add table-driven tests to ensure missing/false/null/string values stay as assistant text while
truebecomes a summary group.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agent/compaction/index.go | Expands _is_summary detection to support raw JSON boolean token representations in message metadata. |
| agent/compaction/compaction_test.go | Adds parity tests covering expected summary vs assistant-text grouping for various _is_summary value forms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestMessageIndex_SummaryPropertyKeyIsRespected(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| value any | ||
| expected compaction.GroupKind | ||
| }{ | ||
| {name: "missing", expected: compaction.GroupKindAssistantText}, | ||
| {name: "bool false", value: false, expected: compaction.GroupKindAssistantText}, | ||
| {name: "raw false", value: json.RawMessage(`false`), expected: compaction.GroupKindAssistantText}, | ||
| {name: "raw null", value: json.RawMessage(`null`), expected: compaction.GroupKindAssistantText}, | ||
| {name: "raw string", value: json.RawMessage(`"Unexpected string"`), expected: compaction.GroupKindAssistantText}, | ||
| {name: "bool true", value: true, expected: compaction.GroupKindSummary}, | ||
| {name: "raw true", value: json.RawMessage(`true`), expected: compaction.GroupKindSummary}, | ||
| } |
API Consistency Review — No Issues FoundThis PR is out of scope for cross-repo parity concerns. Changes reviewed:
Assessment: No exported identifiers, option structs, observable runtime behavior, or example/sample coverage were changed. ✅
|
|
@copilot respond to PR comments |
…opertyValueParsing Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>
Done — addressed both points in commit
All 14 subtests pass. |
Summary
Align Go compaction summary detection with the upstream .NET fix for
_is_summarymetadata by accepting both nativeboolvalues and raw JSON boolean tokens when grouping assistant messages. Added parity tests that keep missing, false, null, and string token values as assistant text while recognizingtruein both native and raw-token forms.Upstream reference:
microsoft/agent-framework@e677ccc
Ported .NET PRs
CompactionMessageIndex.IsSummaryMessagefor deserialized JSON token valuesBreaking Changes
No.
Tests and Examples
go test ./agent/compactionTestMessageIndex_SummaryPropertyKeyIsRespectedNotes
Skipped broader recent upstream compaction/workflow changes because they were either already tracked in Go porting work or were larger than the narrow nightly PR target. This change is internal-only and does not modify exported Go symbols.
Closes #487