Conversation
eanzhao
force-pushed
the
feat/2026-04-21_issue-257-channel-abstractions
branch
from
6032a94 to
3813d44
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## dev #272 +/- ##
=======================================
Coverage 68.58% 68.59%
=======================================
Files 1110 1110
Lines 78074 78074
Branches 10221 10221
=======================================
+ Hits 53549 53551 +2
+ Misses 20614 20612 -2
Partials 3911 3911
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
重新 review 了最新修复，前一轮指出的代码问题都已经补齐：
我重新本地验证了：
以上都通过。 如果按这次 review 明确略过 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36f3bec997
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// This helper always targets <see cref="Activity"/>'s conversation and uses the adapter's bot credential for the | ||
| /// current turn. | ||
| /// </remarks> | ||
| Task<EmitResult> SendAsync(MessageContent content); |
There was a problem hiding this comment.
ITurnContext methods used for outbound I/O (send/reply/update/delete/stream start) do not accept a CancellationToken, so adapters are not contractually required to stop network operations when a turn is canceled (for example request abort, actor shutdown, or timeout). This can leave canceled turns still performing outbound writes and makes graceful shutdown semantics inconsistent across implementations.
Useful? React with 👍 / 👎.
| /// </summary> | ||
| /// <param name="activityId">The opaque activity identifier returned by a previous send or reply.</param> | ||
| /// <returns>The emit result produced by the active adapter.</returns> | ||
| Task<EmitResult> DeleteAsync(string activityId); |
There was a problem hiding this comment.
ITurnContext.DeleteAsync returns EmitResult, but IChannelOutboundPort.DeleteAsync only returns Task, and EmitResult is documented as a send/update outcome type. This mismatch forces turn-context implementations to fabricate delete results (or hide failures behind exceptions only), which makes delete behavior observability inconsistent with the rest of the abstraction surface.
Useful? React with 👍 / 👎.
- Thread CancellationToken through ITurnContext send/reply/update/delete and BeginStreamingReplyAsync so adapters can honor turn cancellation (request abort, actor shutdown, timeout). - Change ITurnContext.DeleteAsync to return Task to match IChannelOutboundPort.DeleteAsync; EmitResult.SentActivityId semantics do not apply to deletes, so implementations no longer need to fabricate emit results. - Update docs/canon accordingly. Addresses Codex P2 review comments on PR #272. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 488cb9ea4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ArgumentNullException.ThrowIfNull(segments); | ||
|
|
||
| var normalized = new List<string> { NormalizeSegment(channel.Value, nameof(channel)) }; | ||
| for (var i = 0; i < segments.Length; i++) |
There was a problem hiding this comment.
BuildCanonicalKey accepts an empty segments array, and ConversationReference.Create exposes that path via optional params, so Create(channel, bot, scope, partition) currently produces a key with only the channel id. That collapses distinct conversations on the same channel onto the same canonical key and can break routing/dedup semantics once a caller omits segments by mistake; this helper should fail fast (or inject required identifiers) when no deterministic segments are provided.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d26dcbe1.
ConversationReference.BuildCanonicalKey(...) now rejects empty segments, and ConversationReference.Create(...) inherits that fail-fast behavior instead of allowing a channel-only canonical key. I also added a protocol test that asserts missing canonical segments throw immediately.
| public static RedactionResult Unchanged(byte[] payload) => | ||
| new(payload ?? throw new ArgumentNullException(nameof(payload)), false); |
There was a problem hiding this comment.
RedactionResult.Unchanged passes the caller’s byte[] through by reference, so if upstream uses pooled buffers or clears/reuses the array after redaction, SanitizedPayload mutates after the fact. That can lead to corrupted persisted payloads and can reintroduce sensitive data handling bugs in ingress flows; this contract should copy bytes (or switch to an immutable buffer type) before storing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d26dcbe1.
RedactionResult now defensively clones the supplied payload bytes before storing them, so later caller-side buffer reuse or mutation cannot change SanitizedPayload after the fact. The protocol tests now cover both Unchanged(...) and Modified(...) for this copy-on-store behavior.
| for path in files: | ||
| text = path.read_text(encoding="utf-8") | ||
| for match in pattern.finditer(text): | ||
| name = match.group(1) |
There was a problem hiding this comment.
The guard evaluates each interface declaration body in isolation, so a partial interface split across files can place runtime methods in one part and outbound methods in another without ever tripping has_runtime_surface && has_outbound_surface. That bypasses the stated CI invariant while still producing a merged mega-interface at compile time; the check should aggregate detected methods by interface name before deciding violations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d26dcbe1.
The guard now aggregates interface surface by interface name across all declaration parts before deciding violations, so a partial interface can no longer split runtime and outbound members across files to bypass the check. I also added a guard-focused test that creates a split partial interface fixture and verifies the script fails.
1 participant
Closes #257
Summary
Aevatar.GAgents.Channel.Abstractionssurface for#257single-class two-interfacescontract via a conformance suite andchannel_mega_interface_guard.shCS1591as an error for the abstractions packageNotes
#256is now merged intodev, so this PR carries only the#257abstractions changesICredentialProvidercontract instead of introducing a channel-local duplicateValidation
dotnet build agents/Aevatar.GAgents.Channel.Abstractions/Aevatar.GAgents.Channel.Abstractions.csproj --nologodotnet test test/Aevatar.GAgents.Channel.Protocol.Tests/Aevatar.GAgents.Channel.Protocol.Tests.csproj --nologobash tools/ci/architecture_guards.sh