feat(colors): add private observation update lifecycle - #571
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughPrivate fixture переведён с ABI v1 на ABI v2. Добавлены явные observation updates, stream, revision и lifecycle-состояния. JavaScript consumer получил метод ChangesABI v2 и observation updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Consumer
participant WasmFixture
participant Attachment
Browser->>Consumer: update(updateBytes)
Consumer->>WasmFixture: update_v2(updateBytes)
WasmFixture->>Attachment: apply observation update
Attachment-->>WasmFixture: result_v2
WasmFixture-->>Consumer: state, stream, revision
Consumer-->>Browser: update receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Measured by GitHub Actions run 31574811224: 318085 raw bytes on linux-x64 with zero headroom. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/colors/private-program/consumer.js (1)
259-283: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winПроверяйте provenance результата перед возвратом receipt.
decodeReceiptвозвращаетstreamиrevision, но не сравнивает их с active attachment.updateпроверяет только stream входящего update. Ошибочный WASM может вернуть stale receipt или receipt другого stream с тем же output identity.Храните последнюю принятую revision. Перед возвратом receipt проверяйте stream и monotonic revision. Добавьте hostile tests для чужого stream и rollback revision.
🤖 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 `@packages/colors/private-program/consumer.js` around lines 259 - 283, Update decodeReceipt and its active-attachment state to retain the last accepted revision, then validate the decoded stream against the active attachment and require revision to be monotonic before returning the receipt. Reject receipts from a different stream or with a rolled-back revision, and add hostile tests covering both foreign-stream and revision-rollback receipts.scripts/test-private-program-mutations.mjs (1)
229-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winПроверяйте тип до нормализации CRLF.
При invalid
sourceвызовsource.replaceAll()выбрасывает обычныйTypeErrorдо предусмотренногоfail. Объектnew String(...)также может пройти нормализацию, хотя вход не является primitive string.Сначала проверьте типы
source,definition.searchиdefinition.replacement. Затем нормализуйте CRLF. Добавьте test дляapplyExactMutation(null, definition).Согласно coding guidelines: «invalid, unreachable, unsupported и incomplete context должны возвращаться типизированно».
Предлагаемое исправление
export function applyExactMutation(source, definition) { - source = source.replaceAll("\r\n", "\n"); if ( typeof source !== "string" || typeof definition?.search !== "string" || typeof definition?.replacement !== "string" ) { fail("mutation source, search anchor, and replacement must be strings"); } + source = source.replaceAll("\r\n", "\n");🤖 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 `@scripts/test-private-program-mutations.mjs` around lines 229 - 236, В функции applyExactMutation сначала проверьте, что source, definition.search и definition.replacement являются primitive string, до вызова source.replaceAll; затем нормализуйте CRLF только после успешной валидации. Добавьте тест для applyExactMutation(null, definition), подтверждающий типизированный fail вместо обычного TypeError, и сохраните существующую обработку корректных входных данных.Source: Coding guidelines
crates/labcolors-core/src/private_fixture.rs (2)
646-655: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winНе возвращайте заглушки
state/stream/revisionизproject_certified_render_v1.Функция заполняет
state: Ready,stream: 0,revision: 0временными значениями. Корректность результата обеспечивается только тем, чтоproject_attachment_commit_v2сразу перезаписывает эти три поля (Line 630-632). Любой новый вызывающий получит молча неверную голову наблюдения.Передавайте
state,streamиrevisionпараметрами, чтобы функция не могла вернуть недостроенный результат.♻️ Предлагаемое изменение
fn project_certified_render_v1( render: crate::program::attachment::AttachedRenderOutputV1<'_, HandoffPointSinkOutputIdV1>, + state: PrivateFixtureStateV2, + stream: u32, + revision: u64, ) -> Result<CertifiedPrivateFixtureResultV1, PrivateFixtureErrorV1> { @@ Ok(CertifiedPrivateFixtureResultV1 { - state: PrivateFixtureStateV2::Ready, - stream: 0, - revision: 0, + state, + stream, + revision, output: render.output().value(),В
project_attachment_commit_v2(Line 629-632) вызов становится единственным источником значений:- let mut result = project_certified_render_v1(render)?; - result.state = state; - result.stream = stream; - result.revision = revision; - Ok(result) + project_certified_render_v1(render, state, stream, revision)🤖 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 `@crates/labcolors-core/src/private_fixture.rs` around lines 646 - 655, Update project_certified_render_v1 to accept state, stream, and revision as parameters and populate those fields directly from the arguments instead of using Ready, 0, and 0 placeholders. Update project_attachment_commit_v2 to pass the authoritative values when calling it, ensuring every caller receives a fully constructed CertifiedPrivateFixtureResultV1.
804-820: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
streamвcomplete_runдублирует источник истины.Attachment уже создан с
authored.streamвexecute_private_fixture_v1(Line 492).complete_runпринимаетstreamотдельным параметром и сохраняет его вActive. Значения могут разойтись: сигнатура допускает любойstream, не связанный с attachment. Расхождение проявится позже как ложныйUpdateRejectedвupdateпри корректном обновлении, либо как принятие обновления с чужим stream.Перенесите
streamвExecutedPrivateFixtureV1, чтобы одно значение фиксировалось в момент создания attachment.♻️ Предлагаемое изменение
struct ExecutedPrivateFixtureV1<H> where H: HandoffPointSinkHostV1, { + stream: u32, attachment: HandoffAttachmentV1<H>, projection: Result<CertifiedPrivateFixtureResultV1, PrivateFixtureErrorV1>, }Затем
complete_runтеряет параметрstreamи читаетexecuted.stream.🤖 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 `@crates/labcolors-core/src/private_fixture.rs` around lines 804 - 820, Перенесите поле stream в структуру ExecutedPrivateFixtureV1 и заполняйте его тем же значением, которое используется при создании attachment в execute_private_fixture_v1. Удалите параметр stream из complete_run и сохраняйте в PrivateFixtureLifecycleV1::Active значение executed.stream, чтобы attachment и состояние использовали единый источник истины.
🤖 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 `@crates/labcolors-core/src/private_fixture.rs`:
- Around line 1247-1259: В функции update_v2 обнулите экспортируемую ячейку
RESULT_V1 до вызова update_request_v2, сохранив публикацию staged_result после
возврата. Приведите порядок действий к контракту run_fixed_buffer_entry_v1:
очистка перед входом в хост, затем запись актуального результата после
завершения update_request_v2.
- Around line 397-437: Расширьте cfg-гейт у decode_update_v2 и связанных с ним
констант, включая PRIVATE_FIXTURE_UPDATE_V2_MAGIC, чтобы код был доступен при
тестовой сборке на нативной цели. Добавьте Rust-тесты для враждебных кадров,
проверяющие отклонение неверных magic, версии и длины, kind 1 с ненулевым reason
и kind 2 с ненулевым хвостом, а также сохраните успешные случаи для валидных
кадров.
- Around line 36-37: Добавьте комментарий непосредственно к MAX_SCENARIOS_V2,
объясняющий инвариант ограничения фикстуры двумя коррелированными сценариями и
то, что при увеличении лимита необходимо обновить wire-грамматику и ветвление в
apply_observed_update_v2.
- Around line 529-549: Replace the unreachable! branch in
apply_observed_update_v2 with a typed error for scenario counts outside
1..=MAX_SCENARIOS_V2, and propagate that error through attachment.update. Remove
the now-redundant map_err handling in execute_private_fixture_v1 and
apply_update_v2 while preserving existing valid one- and two-scenario behavior.
- Around line 1612-1681: Добавьте тесты для оставшихся ветвей update: вызов в
состоянии, отличном от Active, должен возвращать InvalidLifecycle; обновление с
чужим stream должно возвращать UpdateRejected и сохранять опубликованное
состояние; сценарий с двумя элементами должен покрыть ветвь len == 2 в
apply_observed_update_v2. В существующих и новых тестах замените хардкод stream
31 на valid_authored().stream, сохраняя authored как единый источник значения.
- Around line 259-273: Update read_scenarios_v2 so every ScenarioWireV2 slot
beyond len is validated as zero-valued, including both id and backdrop; return
PrivateFixtureErrorV1::InvalidAuthoredData when any unused slot is nonzero,
while preserving the existing validation and parsing for slots within len.
- Around line 604-625: Update the empty-render branch in the certified result
construction to derive admission from the mapped `state`/`StateKindV1` rather
than trusting the caller-provided `admit_empty_lifecycle` flag. Allow the
zero-valued result only for states whose contract permits no certified render,
and continue returning `MissingCertifiedOutput` for `Ready` or any other state
requiring a certificate; adjust `project_update_commit_v2` usage as needed so
the invariant is enforced at this boundary.
---
Outside diff comments:
In `@crates/labcolors-core/src/private_fixture.rs`:
- Around line 646-655: Update project_certified_render_v1 to accept state,
stream, and revision as parameters and populate those fields directly from the
arguments instead of using Ready, 0, and 0 placeholders. Update
project_attachment_commit_v2 to pass the authoritative values when calling it,
ensuring every caller receives a fully constructed
CertifiedPrivateFixtureResultV1.
- Around line 804-820: Перенесите поле stream в структуру
ExecutedPrivateFixtureV1 и заполняйте его тем же значением, которое используется
при создании attachment в execute_private_fixture_v1. Удалите параметр stream из
complete_run и сохраняйте в PrivateFixtureLifecycleV1::Active значение
executed.stream, чтобы attachment и состояние использовали единый источник
истины.
In `@packages/colors/private-program/consumer.js`:
- Around line 259-283: Update decodeReceipt and its active-attachment state to
retain the last accepted revision, then validate the decoded stream against the
active attachment and require revision to be monotonic before returning the
receipt. Reject receipts from a different stream or with a rolled-back revision,
and add hostile tests covering both foreign-stream and revision-rollback
receipts.
In `@scripts/test-private-program-mutations.mjs`:
- Around line 229-236: В функции applyExactMutation сначала проверьте, что
source, definition.search и definition.replacement являются primitive string, до
вызова source.replaceAll; затем нормализуйте CRLF только после успешной
валидации. Добавьте тест для applyExactMutation(null, definition),
подтверждающий типизированный fail вместо обычного TypeError, и сохраните
существующую обработку корректных входных данных.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93753bb4-1422-4eb5-a5b8-9fd99b747efe
📒 Files selected for processing (11)
crates/labcolors-core/src/generic_boundary_tests.rscrates/labcolors-core/src/private_fixture.rsfixtures/private-program-browser/proof.mjspackages/colors/bench/private-program-wasm.jsonpackages/colors/private-program/consumer.jspackages/colors/test/private-program-consumer.test.mjspackages/colors/test/private-program-mutation-contract.test.mjspackages/colors/test/release-contract.test.mjsscripts/build-private-program.mjsscripts/test-private-program-browser.mjsscripts/test-private-program-mutations.mjs
Bind the observed-to-Unknown mutant to the browser lifecycle assertion emitted by the ABI-v2 update path. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Close CodeRabbit findings by removing placeholder state, unifying stream ownership, validating result provenance, eliminating unreachable update states, and preserving typed mutation failures. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
@coderabbitai Addressed the substantive findings in 51be60d: result provenance is stream/revision-bound, placeholder receipt fields are removed, stream ownership has one source, unused scenario slots and terminal state are fail-closed, update result is cleared before host entry, the unreachable branch is typed, and mutation input validation now precedes CRLF normalization. Targeted Rust/JS/clippy gates pass. Please re-review the current head. |
|
🧠 Learnings used
|
Measured by GitHub Actions run 31583435451: 318134 raw bytes on linux-x64 with zero headroom after review fixes. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Validate the total lifecycle/output result grammar and treat every uncertain post-WASM outcome as a poisoned lifecycle requiring cleanup. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Describe legacy-named request/result exports as ABI-v2 framed and identify the dedicated v2 update exports consistently. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
@coderabbitai Blind-auditor F1/F2/F3 are closed in 1aa3f99 and 4356d5e. F1: exact state enum plus total Ready/no-output field invariants with hostile matrix tests. F2: traps, host failures, and post-commit receipt failures now use Core probe + poison cleanup; foreign-stream post-commit test proves no stale reuse. F3: mixed legacy-export/ABI-v2 wording corrected. Targeted JS 52/52, fmt, clippy -D warnings green. Please review current head. |
|
I will verify the F1 state and output invariants, the F2 cleanup paths and stale-reuse prevention, and the F3 ABI wording. I will also consider the existing fail-closed arena-return invariants from the prior review context. 🧠 Learnings used
|
1 participant
Slice contract
PR E lifts runtime observation/update into the existing private Program artifact after PR D's static-first cut.
Included
Observedupdates run a full re-resolve; explicitUnknownnever fabricates a backdrop or certificate.SetAll,RevokeAll, and exact replay. Rejected updates leave the committed Core/session and host publication unchanged.Excluded
RED list
updateoperation (proved red before implementation).Exit criteria
Local verification
Passed:
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo check -p labcolors-core --features private-fixture --testscargo test -p labcolors-core --lib --features private-fixture private_fixture::tests::(19/19)node --test packages/colors/test/private-program-consumer.test.mjs packages/colors/test/private-program-mutation-contract.test.mjs(50/50)Environment-qualified:
Budget cycle
This head intentionally leaves
packages/colors/bench/private-program-wasm.jsonunchanged. The first GitHub Linux run must print the new observed raw bytes and fail the exact ratchet. A follow-up commit will copy only that real run's bytes/source into the zero-headroom budget, then the exact new head must rerun fully green.Summary by CodeRabbit
?????????? ??????????????????????
update?????? ?????????????????? ????????????????????, ?????????????? ?? ??????????????.??????????????????????
Remote verification