feat(watchsync): expose authoritative completion state - #15
Conversation
📝 WalkthroughWalkthroughThe watch sync protobuf now includes a host-determined ChangesWatch completion state
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: f13129fa3f
ℹ️ 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".
| // Authoritative host completion state for playback events. A history ID can | ||
| // exist for incomplete playback, and completion_percent may be below 100 | ||
| // when the host's configured watched threshold has been reached. | ||
| bool completed = 14; |
There was a problem hiding this comment.
A plugin built against this SDK can still run on an older v1 host because silo_api_version is the only runtime compatibility gate, but that host will omit field 14. Since a non-optional proto3 bool decodes both omission and an authoritative false as false, the plugin cannot distinguish an unsupported field from an incomplete play; following the new guidance would therefore misclassify completed plays—especially those below 100%—when paired with an older host. Declare this as optional bool so providers can use the authoritative value when present and retain legacy inference when it is absent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/silo/plugin/v1/watch_sync_provider.proto`:
- Around line 312-315: The completed field’s presence semantics are unspecified.
Determine whether omission differs from false; if so, change completed to
optional bool, regenerate the Go bindings, and update callers to check presence.
Otherwise, document that omission means false and add a compatibility test
covering an omitted completed field.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e3ddb24-4528-48af-aebb-c22648ce5342
⛔ Files ignored due to path filters (1)
pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
README.mdpkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.goproto/silo/plugin/v1/watch_sync_provider.proto
| // Authoritative host completion state for playback events. A history ID can | ||
| // exist for incomplete playback, and completion_percent may be below 100 | ||
| // when the host's configured watched threshold has been reached. | ||
| bool completed = 14; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'completed = 14|Completed\s+(bool|\*bool)|HasCompleted' \
proto/silo/plugin/v1/watch_sync_provider.proto \
pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.go
rg -n -C 5 '\bWatchSyncEvent\s*\{|\bCompleted\s*[:=]' --glob '*.go'Repository: Silo-Server/silo-plugin-sdk
Length of output: 1638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- proto context ---'
sed -n '270,330p' proto/silo/plugin/v1/watch_sync_provider.proto
printf '%s\n' '--- relevant generated type ---'
sed -n '1915,1970p' pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.go
printf '%s\n' '--- completed references ---'
rg -n -C 4 '\b(completed|Completed|GetCompleted|HasCompleted)\b' \
--glob '*.go' --glob '*.proto' --glob '*.md' --glob '*.yaml' --glob '*.yml' .
printf '%s\n' '--- target test ---'
rg -n -C 12 'Completed|completed' pkg/pluginproto/silo/plugin/v1/watch_sync_provider_test.go || trueRepository: Silo-Server/silo-plugin-sdk
Length of output: 28116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- contract documentation ---'
sed -n '120,150p' README.md
rg -n -C 3 'authoritative|completion state|completion_percent|watch_history_id|incomplete playback|omission|absent' \
README.md docs proto pkg --glob '*.md' --glob '*.proto' --glob '*.go' || true
printf '%s\n' '--- field-presence patterns ---'
rg -n -C 3 'optional (bool|int32|int64|string)|\*bool|Has[A-Z][A-Za-z0-9]*\(' \
proto pkg --glob '*.proto' --glob '*.go' || true
printf '%s\n' '--- repository state ---'
git status --short
git diff --statRepository: Silo-Server/silo-plugin-sdk
Length of output: 36954
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
proto = Path("proto/silo/plugin/v1/watch_sync_provider.proto").read_text()
generated = Path("pkg/pluginproto/silo/plugin/v1/watch_sync_provider.pb.go").read_text()
field = re.search(r'(?m)^\s*bool completed = 14;\s*$', proto)
go_field = re.search(
r'(?m)^\s*Completed\s+bool\s+`protobuf:"varint,14,opt,name=completed,proto3"[^`]*`',
generated,
)
assert field and go_field, "completed declarations do not match the expected plain proto3 bool"
assert "optional bool completed = 14;" not in proto
# Proto3 scalar bool encoding: false is omitted; true is field 14 (wire key 14<<3).
def encode_completed(value):
return b"" if not value else bytes([14 << 3, 1])
def decode_completed(data):
return any(data[i] == (14 << 3) and data[i + 1] == 1
for i in range(len(data) - 1))
assert encode_completed(False) == b""
assert decode_completed(encode_completed(False)) is False
assert decode_completed(encode_completed(True)) is True
print("plain proto3 bool: false encodes as omission; absent and false decode to the same value")
PYRepository: Silo-Server/silo-plugin-sdk
Length of output: 253
Document the omission semantics for completed.
completed is a plain proto3 bool, so false is omitted on the wire and an absent field decodes as false. If omission and false differ, change the field to optional bool, regenerate the Go code, and update callers to check presence. Otherwise, document that omission means false and add an omitted-field compatibility test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@proto/silo/plugin/v1/watch_sync_provider.proto` around lines 312 - 315, The
completed field’s presence semantics are unspecified. Determine whether omission
differs from false; if so, change completed to optional bool, regenerate the Go
bindings, and update callers to check presence. Otherwise, document that
omission means false and add a compatibility test covering an omitted completed
field.
1 participant
Summary
completedfield toWatchSyncEventWhy
Incomplete playback can still carry a watch-history ID, while a completed event may be below 100% because the host applies a configurable watched threshold. Providers need the host's explicit decision to preserve watch state correctly.
Validation
go test ./...AI use
Implemented and validated with OpenAI Codex under maintainer direction.
Summary by CodeRabbit
New Features
completedplayback event field that records the host-determined watched status independently of playback history or percentage watched.Documentation
completedas the authoritative completion status and not infer it from other fields.Bug Fixes