Repository navigation
Approve the actor and codec-driver boundary (10.1.3) - #540
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR accepts ADR 010, defines the zero-copy actor and codec-driver boundary, documents compatibility rules and implementation guidance, adds the associated ExecPlan, and marks the relevant roadmap entries complete. ChangesZero-copy transport boundary documentation
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/contents.md`:
- Around line 52-53: The sibling entry wording is inconsistent because the
`10.1.3` link description in `contents.md` is missing the “Completed” prefix.
Update the markdown entry for `Approve actor and codec-driver boundary` so the
visible description matches the completed-state phrasing used by the `10.1.1`
sibling entry, keeping the same link target and aligning the text in the table
of contents.
In `@docs/developers-guide.md`:
- Around line 48-49: Use en-GB-oxendict spelling in the developers guide: update
the “serialization” wording in the sentence about the codec driver and
FrameCodec::wrap_payload to “serialisation” so it matches the project’s spelling
convention; leave the rest of the sentence unchanged and verify only the
noncompliant occurrence in that paragraph.
In `@docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md`:
- Line 155: The ExecPlan uses non-Oxford spellings that conflict with the
en-GB-oxendict style guide; update the affected documentation text in this
markdown file by replacing each -ise form with the corresponding -ize form. In
the ExecPlan content, change the wording around the roadmap completion note and
the other referenced sentences so the spellings in the relevant paragraphs use
finalize, finalizing, parallelize, and parameterizes consistently throughout.
In `@docs/frame-vec-u8-inventory.md`:
- Around line 351-356: The docs text in the frame-vec-u8 inventory note says the
app-router `before_send` gap is “tracked separately” but does not point readers
to the actual tracking item. Update the surrounding wording in this note to
include a direct link or parenthetical citation to the relevant roadmap entry,
issue, or ADR, using the existing `before_send` mention and nearby `roadmap.md`
reference as the anchor so contributors can navigate to the tracking source.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: a5e74d78-0464-455c-b0d7-516ca033ab38
📒 Files selected for processing (7)
docs/adr-010-transport-frame-boundary-for-zero-copy.mddocs/contents.mddocs/developers-guide.mddocs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.mddocs/frame-vec-u8-inventory.mddocs/roadmap.mddocs/zero-copy-frame-and-payload-migration-roadmap.md
Draft the decision-closure ExecPlan for roadmap item 10.1.3, which approves ADR 010 by resolving its three Outstanding Decisions: protocol hooks stay packet-oriented (app-router before_send gap tracked to 11.2.1), no public Vec<u8> compatibility shim is added (CorrelatableFrame for Vec<u8> leaves the core surface under the breaking release in 11.2.2), and no new "serializable packet" trait is introduced (Packet + EncodeWith suffices, corroborated by tonic and tokio_util prior art). The plan is documentation-only; runtime work is deferred to phase 11. It records a Logisphere "proceed with conditions" review and the sibling-item delivery pattern from 10.1.1/10.1.2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mark the approved execplan as in progress and record the preflight findings used before editing ADR 010.
Move ADR 010 to Accepted and resolve its protocol hook, Vec<u8> bridge, and serializable packet trait decisions.
Mark the actor and codec-driver boundary decision complete across the roadmaps, inventory, developers' guide, and documentation index.
Record completed ADR and propagation stages plus the refined stale-reference validation note before the full gates.
Record final gate results, CodeRabbit findings, and retrospective for the ADR 010 decision-closure work.
Align the completed plan index, Oxford spelling, roadmap traceability, and ExecPlan privacy wording.
315844a to
15790e8
Compare
Align the ADR 010 documentation with the repository spelling policy and Markdown lint rules.
|
@coderabbitai Have the following now been resolved? ❌ Failed checks (1 error)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== ExecPlan outline =='
ast-grep outline docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md --items all || true
echo '== Absolute home-path markers in changed documentation =='
rg -n -i -C 3 \
'(/home/|/Users/|[A-Z]:\\Users\\|\.lody(?:/|\\)|/root/)' \
docs/adr-010-transport-frame-boundary-for-zero-copy.md \
docs/contents.md \
docs/developers-guide.md \
docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md \
docs/frame-vec-u8-inventory.md \
docs/roadmap.md \
docs/zero-copy-frame-and-payload-migration-roadmap.md || true
echo '== Planning-agent/session/operational metadata markers in ExecPlan =='
rg -n -i -C 3 \
'(planning[ -]?agent|agent[ -]?(metadata|session)|session[ _-]?(id|metadata)|\.lody|workspace|transcript|prompt|model|run[ _-]?id|execution[ _-]?id)' \
docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md || true
echo '== ExecPlan references to local/repository paths =='
rg -n -C 2 '(?:^|[^[:alnum:]_])(docs|src|tests)/[[:alnum:]_./-]+' \
docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md || trueLength of output: 12649 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Execution and retrospective sections =='
sed -n '430,620p' docs/execplans/10-1-3-approve-actor-and-codec-driver-boundary.md
echo '== Broad sensitive-token scan across the current PR diff =='
git diff --unified=0 "$(git merge-base HEAD origin/main)" HEAD -- docs |
rg -n -i -C 2 \
'(/home/|/Users/|[A-Z]:\\Users\\|\.lody|planning[ -]?agent|agent[ -]?session|session[ _-]?(id|metadata)|run[ _-]?id|execution[ _-]?id|api[_ -]?key|token|password|secret)' || trueLength of output: 9179
The absolute local-path exposure is resolved: the current PR documentation contains no Remove the remaining planning/review-agent operational metadata from
Keep repository-relative paths and generic |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Keep the ADR 010 decision conditions while removing review tooling and reviewer persona references.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai stop asking for markdown documents to be split. The 400 line rule applies only to code. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✏️ Learnings added
|
Address review feedback on the 10.1.3 ExecPlan's stale-reference scan: - Restrict the preserved dual-date exclusion to the ADR 010 document path by anchoring it to `adr-010-...md:<line>:` in the ripgrep filter, instead of matching the dual-date text in any document. - The exact dual-date proposal-history line is still excepted within the ADR, but the same "Proposed .. Accepted .." text in any other governing document now remains detectable as drift. - Clarify the accompanying comment to state the exception is scoped to the ADR's own path. Docs-only. make check-fmt, lint, typecheck, test, and markdownlint pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces this repository's Rust copy of the CV-005 contract with `cv005-contracts check` from leynos/shared-actions, pinned to a full commit (a38feb9b, the merge of shared-actions #540) in `CV005_CONTRACTS_REF`. The repository's only parameter is `repository` in `.github/cv005.toml`. `make test-workflow-contracts` runs the CLI, and CI runs that target in a "Check the CV-005 contracts" step, because the deleted cargo tests were the only thing running the contract there. Removed from `tests/workflow_contracts/`: the repository's copy of the CV-005 contract: `ci_codescene_placement_test.py`, `codescene_environment.py` and its test, `codescene_placement_policy.py`, `codescene_placement_reader.py` and its test, `codescene_publisher_test.py`, `codescene_retired_variable_test.py`, `workflow_calls.py` (the classification of local calls, used only to build the closure), and `coverage_main_workflow_test.py`, whose clauses on the publisher (full-commit uploader pin, token handling) are the library's. The runner-placement, Whitaker-cache, dev-fast, Loom, mutation-testing, concurrency and shared-actions-pin contracts stay. The one thing they took from the deleted modules, the trigger reader the concurrency contract uses, is kept and renamed for what it is: `codescene_placement_reader.py` becomes `workflow_reading.py` (only `triggers`; its closure, secret and mention readers go) and its test becomes `workflow_reading_test.py` with the two trigger cases. The target already ran in CI as its own step, so no CI step is added. The library found nothing to fix in the workflows, so this is a pure deletion. Proof: - `make test-workflow-contracts` passes (the library, then 186 pytest cases). - Setting `cancel-in-progress: true` on the publisher fails it with `publisher.concurrency`; removing its `persist-credentials: false` fails it with `publisher.least-privilege`. - `make check-fmt` and `make markdownlint` pass. Rust gates are untouched by this change and run in CI. The developers' guide's contract section now describes the library, the `CV005_CONTRACTS_REF` pin (a full commit; a fix is a pin bump), `.github/cv005.toml`, and the `uv` prerequisite. ## Summary by Sourcery Replace the local CV-005 contract copy with the pinned shared contract library while preserving repository-specific workflow contracts. New Features: - Run the shared CV-005 contract library against the repository’s workflows via the workflow-contract test target. Enhancements: - Remove the repository-owned CodeScene contract implementation and retain only the shared workflow-reading utilities needed by local contracts. - Pin the shared CV-005 contract source to a full commit and configure the repository through `.github/cv005.toml`. Build: - Add Makefile configuration and invocation for the pinned `cv005-contracts` CLI. Documentation: - Update the developers’ guide to document the shared CV-005 library, version pinning, repository configuration, and `uv` prerequisite. Tests: - Rename and retain trigger-reading tests while removing tests covered by the shared CV-005 contract suite.
Summary
Roadmap item
10.1.3is complete. This PR accepts ADR 010: transport-frame boundary for zero-copy so thatVec<u8>bridges leave the core runtime deliberately in phase 11 rather than incidentally.Implemented documentation changes:
ProposedtoAccepted, with dual proposed/accepted dates;Vec<u8>bridge, and serializable-packet-trait decisions in place;FrameCodec::wrap_payloadcall-site inventory and deferred guard coverage to roadmap item11.2.3;10.1.3and zero-copy migration item1.1.3complete;Decision outcome
ADR 010 adopts Option B:
Envelopebefore serialization. The known app-routerbefore_sendgap is documented and tracked under11.2.1.Vec<u8>compatibility shim is introduced.Packet for Vec<u8>remains test-only, and publicCorrelatableFrame for Vec<u8>removal is sequenced with11.2.2/14.2.1.PacketplusEncodeWith<Serializer>already expresses the requirement.Validation
make markdownlintmake check-fmtmake lintmake testcoderabbit review --agentcompleted withfindings: 0References
10.1.3indocs/roadmap.md