feat: run reference status through the module contract - #13
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements the shell-to-module RPC contract end to end: a Protobuf schema, codec, and length-delimited framing; an SDK module server, result type, and in-process testkit; a shell-side RPC launcher/session that spawns and communicates with module processes; shell dispatch wiring that routes resolved modules through invocation and renders table/JSON output; a reference module implementing a Sequence Diagram(s)sequenceDiagram
participant Shell
participant Launcher
participant ModuleProcess as Reference Module
participant Session
Shell->>Launcher: invokeModule(namespace, resolved, args)
Launcher->>ModuleProcess: start executable, sanitized environment
Launcher->>Session: Run(stdin, stdout, invocation)
Session->>ModuleProcess: Hello
ModuleProcess-->>Session: Hello (identity, versions, capabilities)
Session->>ModuleProcess: Welcome (invocation id)
Session->>ModuleProcess: Invoke (status command)
ModuleProcess-->>Session: terminal Result or Problem
Session-->>Launcher: Outcome
Launcher-->>Shell: Outcome + bounded Diagnostics
Shell->>Shell: render table or JSON
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
axewilledge
previously approved these changes
Jul 27, 2026
kanushka
dismissed
axewilledge’s stale review
July 27, 2026 11:09
The merge-base changed after approval.
axewilledge
previously approved these changes
Jul 27, 2026
kanushka
dismissed
axewilledge’s stale review
July 27, 2026 11:16
The merge-base changed after approval.
Closes wso2#5. Plan increment 3 of the architecture proof (wso2#3): `wso2 reference status` now traverses the real shell-to-module boundary and returns a typed result the shell alone renders. The authentication broker and the local status service remain out of scope, so the status handler answers with static semantic data. The module contract schema lives in `sdk/proto/wso2/cli/module/v1`, and its generated Go types are committed to `sdk/protocol/contractv1`, so a clean checkout builds and tests without a Protobuf toolchain. `scripts/generate- protobuf.sh` regenerates them. `sdk/protocol` frames envelopes as an unsigned-varint length followed by one encoded message, rejecting truncated, oversized, and undecodable frames while tolerating unknown fields. `sdk/module` serves the contract for a module author, `sdk/result` carries semantic fields in presentation order, and `sdk/testkit` drives a module through the contract without a subprocess. `internal/rpc` splits the boundary in two. `Session` speaks the protocol over a pair of streams: it proves the running module's namespace, version, and protocol match the receipt, fails closed on a required capability the shell does not provide, binds every post-handshake message to one invocation identifier, and accepts exactly one terminal message with nothing after it. `Launcher` owns the process: it launches only the executable the receipt selected, with an environment built from nothing rather than inherited, bounds standard error as diagnostics, and terminates a module that stops answering. `internal/output` renders both the table and the JSON object from the same ordered fields, so the two cannot disagree, and diagnostics always travel on standard error so structured output stays parseable.
Review follow-up to wso2#5. The translation between the contract's wire types and the Go types both sides work with was written three times, in the shell session, the SDK serve loop, and the test kit. A shell that decoded a result differently from the module that encoded it would be a silent compatibility break, so `sdk/protocol` now owns the pair and everything calls it. `rpc.OutputMode` went with it: the contract's own type is the one both sides should agree on, and the shell maps its rendering choice onto it once. `internal/rpc` had two overlapping paths for finishing with a child process, each closing the module's input and running its own grace timer. They are now one bounded `stop`, which also covers the case that motivated the split: a module blocked writing to a standard output nobody reads any more. `Shell.Context` was never set by any caller, so it is gone; the shell passes the process context directly. Also, from the review: - the plan and a boundaries test now state and enforce that `go.work` may carry exactly one replacement, for the unpublished SDK version the reference module requires, rather than leaving the relaxation to a note in CONTRIBUTING; - the handshake documents why a runtime protocol version is confirmed against the receipt rather than renegotiated: letting the executable widen what its installation declared is the drift the receipt exists to prevent; and - a test covers a module that prints for a user instead of framing a message, which is refused without reaching the user's terminal.
kanushka
force-pushed
the
feat/reference-status-slice
branch
from
July 27, 2026 11:24
772fde3 to
70c8ec5
Compare
Review follow-up to wso2#5. Increment 3's gate requires that crash, non-zero exit, and timeout produce stable shell problems. The launcher tests proved the problem each one becomes, but no black-box run checked what a user or a script actually sees, and the hanging-module case asserted only that the shell stopped eventually. It could have stopped with the wrong exit class, or with half a table on standard output, and passed. The built shell is now run against a module that never answers, one that crashes before answering, and one that answers and then exits uncleanly. Each asserts the exit class, the problem code, and that standard output stays empty, and the crashing case also proves the module's own diagnostics survive the failure that ended the invocation. The acceptance module gained an `exit-uncleanly` control file to make the third case possible. It is read from beside the executable, as the launcher's fake module already does, because the shell passes a module no arguments and no environment; writing the file also leaves the executable unchanged, so its receipt digest still matches. Also from the review: - the claim that the Go tool cannot build this workspace graph without the `go.work` replacement was stated as a general fact about the tool. The Go modules reference documents what a `go.work` replacement is for, but not that, so the plan and CONTRIBUTING now cite the reference for the mechanism and state the rest as what it is: an observation about this checkout, pinned by a boundary test. - `result.Field.Value` records why every value is a string, what that costs, and that typing them is a protocol change for a later slice. The same note is in the schema, so the generated types carry it too.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/rpc/session.go (1)
170-189: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider clamping the deadline before narrowing to
uint32.
invocation.timeout().Milliseconds()is anint64narrowed touint32for the wire.Invocation.Timeoutis exported and currently unvalidated, so a sufficiently large timeout (>~49.7 days) would silently wrap, breaking the stated invariant that the module is told the deadline it will actually be held to.♻️ Optional clamp
+const maxDeadlineMillis = math.MaxUint32 + func (s Session) sendInvoke(writer *protocol.Writer, invocation Invocation) error { + deadline := invocation.timeout().Milliseconds() + if deadline > maxDeadlineMillis { + deadline = maxDeadlineMillis + } return s.write(writer, "the invocation", &contractv1.Envelope{ InvocationId: s.InvocationID, Message: &contractv1.Envelope_Invoke{Invoke: &contractv1.Invoke{ Namespace: invocation.Namespace, CommandPath: invocation.Command, Arguments: invocation.Arguments, OutputMode: protocol.EncodeOutputMode(invocation.OutputMode), Policy: &contractv1.InvocationPolicy{ - DeadlineMillis: uint32(invocation.timeout().Milliseconds()), + DeadlineMillis: uint32(deadline), Interactive: invocation.Interactive, },🤖 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 `@internal/rpc/session.go` around lines 170 - 189, Update sendInvoke to clamp invocation.timeout().Milliseconds() to the uint32 wire range before assigning InvocationPolicy.DeadlineMillis. Preserve the actual timeout for values within range and cap larger values at the maximum representable uint32 value, preventing narrowing from wrapping.Source: Linters/SAST tools
🤖 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 `@sdk/protocol/codec.go`:
- Around line 186-193: Update EncodeVersions to skip any version greater than
the uint32 maximum, in addition to existing non-positive values, before
converting and appending it. Add coverage in codec_test.go for the uint32
boundary and an out-of-range value.
In `@sdk/protocol/framing_test.go`:
- Around line 113-125: Update TestAFrameAtTheSizeLimitIsAccepted so its
constructed envelope serializes to exactly protocol.MaxFrameBytes, adjusting the
payload length as needed rather than using the fixed -16 estimate. Replace the
current upper-bound check with an equality assertion before performing the
round-trip validation.
In `@sdk/protocol/framing.go`:
- Around line 72-82: Update the length-reading logic around binary.ReadUvarint
to track whether any prefix byte was consumed; preserve io.EOF only when the
stream ends before reading a byte, and return io.ErrUnexpectedEOF when EOF
follows a partial varint prefix. Keep other error mappings unchanged.
---
Nitpick comments:
In `@internal/rpc/session.go`:
- Around line 170-189: Update sendInvoke to clamp
invocation.timeout().Milliseconds() to the uint32 wire range before assigning
InvocationPolicy.DeadlineMillis. Preserve the actual timeout for values within
range and cap larger values at the maximum representable uint32 value,
preventing narrowing from wrapping.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 38efbc6e-3500-411c-9c96-fbf460d02c0d
⛔ Files ignored due to path filters (3)
go.workis excluded by!**/*.worksdk/go.sumis excluded by!**/*.sumsdk/protocol/contractv1/contract.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
CONTRIBUTING.mddocs/plans/first-cli-vertical-slice.mdexamples/reference-module/cmd/wso2-module-reference/main.goexamples/reference-module/go.modgo.modinternal/app/app.gointernal/app/app_test.gointernal/app/invoke.gointernal/app/invoke_test.gointernal/boundaries/boundaries_test.gointernal/output/diagnostics.gointernal/output/result.gointernal/output/result_test.gointernal/rpc/diagnostics.gointernal/rpc/invocation_id.gointernal/rpc/launch.gointernal/rpc/launch_test.gointernal/rpc/rpc.gointernal/rpc/session.gointernal/rpc/session_test.gointernal/rpc/testdata/fakemodule/main.goscripts/apply-license-header.shscripts/generate-protobuf.shsdk/buf.gen.yamlsdk/buf.yamlsdk/go.modsdk/module/module.gosdk/module/serve.gosdk/module/serve_test.gosdk/proto/wso2/cli/module/v1/contract.protosdk/protocol/codec.gosdk/protocol/codec_test.gosdk/protocol/framing.gosdk/protocol/framing_test.gosdk/result/result.gosdk/result/result_test.gosdk/testkit/testkit.gotest/acceptance/acceptance_test.gotest/acceptance/status_test.gotest/acceptance/testdata/noisymodule/main.go
Comment on lines
+186
to
+193
| func EncodeVersions(versions []int) []uint32 { | ||
| encoded := make([]uint32, 0, len(versions)) | ||
| for _, version := range versions { | ||
| if version <= 0 { | ||
| continue | ||
| } | ||
| encoded = append(encoded, uint32(version)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline sdk/protocol/codec.go --match EncodeVersions --view expanded
sed -n '183,195p' sdk/protocol/codec.go
rg -n --type go -C 2 '\bEncodeVersions\s*\(' .Repository: wso2/wso2-cli
Length of output: 2350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codec.go relevant section =="
sed -n '160,205p' sdk/protocol/codec.go
echo
echo "== serve.go usage =="
sed -n '135,160p' sdk/module/serve.go
echo
echo "== decode of ProtocolVersions =="
rg -n --type go -C 6 'ProtocolVersions' sdk/protocol sdk/codec sdk/module 2>/dev/null || true
echo
echo "== deterministic Go-like narrowing probe =="
python3 - <<'PY'
u32max = 2**32 - 1
for version in [0, 1, 42, u32max, u32max + 1]:
encoded = version & u32max
print(f"version={version} encoded={encoded} round_trips={encoded == version}")
PYRepository: wso2/wso2-cli
Length of output: 13026
Reject protocol versions outside uint32.
EncodeVersions stores to Hello.ProtocolVersions as []uint32; any int above uint32 max is silently wrapped before being advertised. Skip values above uint32 max alongside non-positive values, and cover the boundary in sdk/protocol/codec_test.go.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 191-191: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(version)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 `@sdk/protocol/codec.go` around lines 186 - 193, Update EncodeVersions to skip
any version greater than the uint32 maximum, in addition to existing
non-positive values, before converting and appending it. Add coverage in
codec_test.go for the uint32 boundary and an out-of-range value.
Source: Linters/SAST tools
Comment on lines
+113
to
+125
| func TestAFrameAtTheSizeLimitIsAccepted(t *testing.T) { | ||
| // The limit is inclusive, so a message of exactly the maximum size is a | ||
| // valid frame rather than a boundary failure. | ||
| large := &contractv1.Envelope{Message: &contractv1.Envelope_Problem{Problem: &contractv1.Problem{ | ||
| Message: strings.Repeat("d", protocol.MaxFrameBytes-16), | ||
| }}} | ||
| encoded, err := proto.Marshal(large) | ||
| if err != nil { | ||
| t.Fatalf("marshalling: %v", err) | ||
| } | ||
| if len(encoded) > protocol.MaxFrameBytes { | ||
| t.Fatalf("the fixture is %d bytes, which exceeds the %d-byte limit", len(encoded), protocol.MaxFrameBytes) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the actual inclusive frame limit.
The fixture only asserts that its encoded size is at most MaxFrameBytes; it is smaller than the limit due to the fixed -16 payload adjustment. Construct or adjust the payload until len(encoded) == protocol.MaxFrameBytes, then assert equality before round-tripping it.
🤖 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 `@sdk/protocol/framing_test.go` around lines 113 - 125, Update
TestAFrameAtTheSizeLimitIsAccepted so its constructed envelope serializes to
exactly protocol.MaxFrameBytes, adjusting the payload length as needed rather
than using the fixed -16 estimate. Replace the current upper-bound check with an
equality assertion before performing the round-trip validation.
Comment on lines
+72
to
+82
| length, err := binary.ReadUvarint(r.source) | ||
| if err != nil { | ||
| if errors.Is(err, io.EOF) { | ||
| // ReadUvarint reports a plain EOF only when no byte of the | ||
| // length was read; a partial length is a damaged frame. | ||
| return nil, io.EOF | ||
| } | ||
| if errors.Is(err, io.ErrUnexpectedEOF) { | ||
| return nil, io.ErrUnexpectedEOF | ||
| } | ||
| return nil, fmt.Errorf("%w: unreadable length prefix: %w", ErrMalformedFrame, err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Distinguish a truncated length prefix from clean EOF.
binary.ReadUvarint returns io.EOF for a prefix such as 0x80 followed by stream end. Lines 74-77 therefore report a clean EOF, so TestATruncatedLengthPrefixIsRejected fails. Track whether a varint byte was consumed and return io.ErrUnexpectedEOF after a partial prefix.
🤖 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 `@sdk/protocol/framing.go` around lines 72 - 82, Update the length-reading
logic around binary.ReadUvarint to track whether any prefix byte was consumed;
preserve io.EOF only when the stream ends before reading a byte, and return
io.ErrUnexpectedEOF when EOF follows a partial varint prefix. Keep other error
mappings unchanged.
axewilledge
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5. Plan increment 3 of the architecture proof (#3): the third increment
of the vertical-slice plan.
What this adds
wso2 reference statusnow traverses the real shell-to-module boundary. Theshell resolves the active module receipt, rechecks the executable digest,
launches one short-lived process, negotiates the module contract, invokes the
status handler, receives a typed outcome, and owns every rendered byte and exit
code. The authentication broker and the local status service are out of scope,
so the status handler answers with static semantic data.
The contract schema.
sdk/proto/wso2/cli/module/v1/contract.protodefinesversion 1 of the module contract. Its generated Go types are committed to
sdk/protocol/contractv1, so a clean checkout builds and tests without aProtobuf toolchain and without network access;
scripts/generate-protobuf.shregenerates them through a pinned
buf.The public SDK.
sdk/protocolframes envelopes as an unsigned-varint lengthfollowed by one encoded message, rejecting truncated, oversized, and undecodable
frames while tolerating unknown fields, and owns the single translation between
wire types and Go types that both sides of the contract share.
sdk/moduleserves the contract for a module author,
sdk/resultcarries semantic fields inpresentation order, and
sdk/testkitdrives a module through the contractwithout a subprocess.
The shell.
internal/rpcsplits the boundary so each half is testablealone.
Sessionspeaks the protocol over a pair of streams: it proves therunning module's namespace, version, and protocol match the receipt, fails
closed on a required capability the shell does not provide, binds every
post-handshake message to one invocation identifier, and accepts exactly one
terminal message with nothing after it.
Launcherowns the process: it launchesonly the executable the receipt selected, with an environment built from nothing
rather than inherited, bounds standard error as diagnostics, and terminates a
module that stops answering.
internal/outputrenders the table and the JSONobject from the same ordered fields, so the two cannot disagree, and diagnostics
always travel on standard error so structured output stays parseable.
Decisions worth a reviewer's attention
go.worknow carries onereplacedirective. Once the SDK gained adependency of its own, the Go tool could no longer build the workspace module
graph without resolving the
sdk v0.0.0that the reference module requires. Thereplacement names that single placeholder version, lives in
go.workratherthan any
go.mod, and disappears when an SDK version is published. This is adeviation from plan section 4 as originally written: the plan is amended in this
pull request, and a new boundaries test pins
go.workto exactly that one lineso nothing else can be added beside it.
Runtime protocol negotiation confirms rather than reopens. The version is
selected during resolution from the receipt and the shell's own list; the
handshake checks that the running module offers it. A module that offers a newer
mutually supported version at runtime is deliberately not negotiated up to it,
because that would let an executable widen what its installation declared, which
is the drift the receipt exists to prevent. A newer version becomes available by
reinstalling.
Required capabilities are enforced shell-side only. The fail-closed path
works and is tested, but the SDK gives a module no way to declare a required
capability, because none exists yet. Adding the field before there is a
capability to name would be speculative.
Verification
Full suite green with
-count=1and under-race, plus the SDK standalone withGOWORK=off. Two real defects surfaced during the work and are fixed andcovered: an unbounded
Waitthat a module blocked on an unread standard outputcould hang the shell on, and a broken-pipe race in the handshake.
Coverage added at every layer of plan section 10: frame codec, negotiation,
result and problem semantics, and rendering as unit tests; handshake,
invocation, malformed frames, and bounded diagnostics as contract tests through
sdk/testkit; sanitized subprocess launch, crash, unclean exit, and timeout asintegration tests against a scriptable stand-in module; and table output, JSON
output, JSON validity while the module is writing diagnostics, protocol
mismatch, and a module that never answers as black-box runs of the built shell
against the built module in an isolated state root.