Skip to content

Fix conversation append capability checks - #565

Closed
senamakel wants to merge 1 commit into
mainfrom
fix/conversation-stream-capability
Closed

senamakel wants to merge 1 commit into
mainfrom
fix/conversation-stream-capability

Conversation

@senamakel

Copy link
Copy Markdown
Member

Summary

DriverConversationStore::admit_append now checks ValidatedBatch on the document port and ConditionalStreamAppend on the stream port. The previous check queried both capabilities from documents, which rejected valid backends whose stream port provides conditional append independently.

Added a regression test with a document port that does not advertise stream capabilities while the stream port does. The test failed before the fix and passes after it.

Verification

  • cargo fmt --all --check
  • cargo test --manifest-path Cargo.toml -p tinyagents-session append_requires_conditional_stream_support_from_the_stream_port -- --exact threads::driver::append_tests::append_requires_conditional_stream_support_from_the_stream_port --nocapture
  • cargo nextest run --profile ci --no-fail-fast --manifest-path Cargo.toml -p tinyagents-session --all-targets (506 passed)

Co-authored-by: Medulla <medulla@tinyhumans.ai>
@tinysweeper

tinysweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Reviewed head: 0a52a13f1aab925aca41287dfd23581c40bf3019

Findings

No active actionable findings.

Notes

Unverified note — critique

  • medium: Test withholds the capability from the wrong port and asserts success (crates/tinyagents-session/src/threads/driver_append_tests.rs:139)
    Problem: `without_document_capability` installs `capabilities: Some(store.docs.capabilities().without(capability))` on the *document* port; the stream port is passed through unchanged (`str
    Suggested fix:
    No concrete fix was supplied.
  • medium: Pincer la vérification sur le port streams, pas sur docs (crates/tinyagents-session/src/threads/driver_append_tests.rs:139)
    Problem: Le changement de `append.rs` déplace la vérification de `Capability::ConditionalStreamAppend` de `self.docs` vers `self.streams`. Le test ne l'exerce pas : il retire la capability
    Suggested fix:
    No concrete fix was supplied.

Verdict: Ready for maintainer review · Priority: medium
Coverage: Reviewed 3 of 3 files.

Diagnostics and supporting details

Full summary

Pull request purpose (title): Fix conversation append capability checks. Tiny Sweeper completed its review; deterministic results follow.

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 0
Tests 2 Noted findings 2
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

Whole pull request behavior: `admit_append` now validates append capabilities in two separate checks, requiring `ValidatedBatch` on `self.docs` and `ConditionalStreamAppend` on `self.streams`; before, both were required on `self.docs`. A new test fixture, `without_document_capability`, wraps the store with a `Documents` double whose advertised capabilities are replaced while the stream port is left unchanged. The added test builds a store whose document port lacks `ConditionalStreamAppend` but whose stream port still advertises it, asserts those capability states, and then asserts that `append_message` succeeds. The tests lane reports that this assertion is also satisfied by the old code and that no added test makes append fail when the stream port lacks `ConditionalStreamAppend`, leaving the behavioral change uncovered. Latest-push delta: unavailable; the review state has no trusted base-to-head diff comparison, so no push-delta changes can be described.

Features

  • Modified — Split append capability validation across document and stream ports: `ValidatedBatch` remains required on the document port, while `ConditionalStreamAppend` is now required on the stream port. This allows backends whose stream port supports conditional append independently of the document port's advertised capabilities, instead of rejecting them based on the document port alone. (crates/tinyagents-session/src/threads/driver/append.rs, crates/tinyagents-session/src/threads/driver/append.rs impl DriverConversationStore {)
  • Added — Regression test for append succeeding when only the document port lacks ConditionalStreamAppend: The test removes `ConditionalStreamAppend` from the document port, verifies that the document port still has `ValidatedBatch` and lacks `ConditionalStreamAppend`, verifies that the stream port retains `ConditionalStreamAppend`, and asserts that `append_message` succeeds. The tests lane reports that this does not distinguish the new behavior from the old behavior. (crates/tinyagents-session/src/threads/driver_append_tests.rs)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Critique findings were capped at Likely confidence because retrieval was degraded: Test withholds the capability from the wrong port and asserts success. (1 model finding retained at no more than Likely confidence because retrieval was degraded) _The code index is behind this pull request (indexed at `63a814ea513e`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The security lane answered with no security or correctness defect in these files: it reports that append capability validation is tightened rather than loosened, with `ValidatedBatch` still checked on the document port and `ConditionalStreamAppend` now checked on the stream port that performs the conditional append; the new fixture hook only affects test doubles.
  • Lane summary: The change tightens append capability validation rather than loosening it: `ValidatedBatch` is still checked on the document port and `ConditionalStreamAppend` is now checked on the stream port, which is the port that actually performs the conditional append. The new fixture hook only affects test doubles and the added test asserts the document port's stale capability mask is ignored. I found no security or correctness defect in these files. _The code index is behind this pull request (indexed at `63a814ea513e`), so retrieved context may be out of date._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Seule modification comportementale : la vérification `ConditionalStreamAppend` porte désormais sur `self.streams` au lieu de `self.docs`. Le nouveau test ne couvre pas ce comportement : il retire la capability du port documents (qui n'est plus consulté) et vérifie que `append_message` réussit — l'assertion centrale passe aussi bien avec l'ancien code qu'avec le nouveau. Aucun test du diff ne fait échouer un append dont le port streams n'a pas `ConditionalStreamAppend`, si bien qu'une régression ramenant la vérification sur `docs`, ou la supprimant, passerait inaperçue. _The code index is behind this pull request (indexed at `63a814ea513e`), so retrieved context may be out of date._

commits

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane answered that there is no sensitive content in the committed files.
  • Lane summary: No sensitive content in committed files. _The code index is behind this pull request (indexed at `63a814ea513e`), so retrieved context may be out of date._

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The description lane answered that the diff matches the description: `admit_append` now asks the stream port for `ConditionalStreamAppend` instead of the document port, and the fixture/test pair exercises a document port that drops that capability while the stream port keeps it; the description is accurate and mentions the significant changes.
  • Lane summary: The diff does what the description says: `admit_append` now asks the stream port for `ConditionalStreamAppend` instead of the document port, and the fixture/test pair exercises a document port that drops that capability while the stream port keeps it. The description is accurate and mentions the significant changes; I found no defect worth reporting. _The code index is behind this pull request (indexed at `63a814ea513e`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.000738
  • Tokens: 225647 input · 13745 output · 38016 cached · 158 embedding
  • Updated: 2026-10-23T02:39:32Z
Head State Pass summary
0a52a13f1aab ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-23T02:39:32Z)

tinysweeper 0.1.2

@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 3 billable files and costs up to $0.75.

Or wait 11 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1241b5d2-51c4-40ba-b05f-5b768c5333ff
📥 Commits

Reviewing files that changed from the base of the PR and between b0ca081 and 0a52a13.

📒 Files selected for processing: 3
  • crates/tinyagents-session/src/threads/driver/append.rs
  • crates/tinyagents-session/src/threads/driver_append_fixtures.rs
  • crates/tinyagents-session/src/threads/driver_append_tests.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T02:39:42.993333Z 0a52a13 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0007

@tinysweeper tinysweeper Bot added the priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later. label Oct 23, 2026
@senamakel

Copy link
Copy Markdown
Member Author

Closing as superseded by #572 (merged to main in 540997d). #572 makes the same fix: admit_append now checks ValidatedBatch on the document port and ConditionalStreamAppend on the stream port. It also adds the regression this PR proposed (an_append_admits_a_stream_capability_its_document_port_does_not_list, a document port without the capability over a stream port that has it) plus a negative test (an_append_is_refused_when_the_stream_port_lacks_conditional_appends). Merging this branch would only add a second fixture and a duplicate test for the same case.

@senamakel senamakel closed this Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant