Repository navigation
apply: extract internal/apply package; wire MCP apply / status / diagnose - #157
Merged
Merged
Conversation
…nose Closes the three biggest "MCP returns NOT_IMPLEMENTED / state-only subset" gaps from PR tronprotocol#153 by hoisting the deploy + probe primitives out of cmd/apply.go and cmd/status.go into a new internal/apply package. cmd/* becomes a thin presentation layer that wraps the same calls MCP and recipe runners now make directly. internal/apply/apply.go Apply(ctx, Options) (*Result, error) — pure-function deploy: render → runtime → state save → optional wait. Includes no-op short-circuit on hash match. internal/apply/wait.go WaitForReady(ctx, target, name, port, timeout) — extracted from cmd/apply.go::waitForNodeReady. internal/apply/probe.go LiveStatus(ctx, target, node) — extracted from cmd/status.go::liveStatusProbe. Returns block_height / is_synced / peer_count via docker-exec curl. internal/apply/apply_test.go 4 tests covering no-op short-circuit, input validation, and IntentHashFromBytes stability against the stdlib SHA256. The cmd-side changes: - cmd/apply.go::runApply shrinks from ~220 lines to ~110; the deploy phase lives in apply.Apply, the cmd layer keeps lock / HUMAN_REQUIRED gate / output formatting. Backward-compatible JSON shape preserved (legacy "status" / "changes" keys still emitted). - cmd/status.go::liveStatusProbe shrinks to a 10-line wrapper that just resolves the target and hands off. - cmd/plan.go's local sha256hex helper replaced with apply.IntentHashFromBytes — same result, one source of truth. The MCP-side wins (the whole point of the extraction): - apply tool now actually deploys instead of returning NOT_IMPLEMENTED_VIA_MCP. The previous test that asserted NOT_IMPLEMENTED is replaced with TestApply_RejectsBadIntentPath which exercises the validation path without needing Docker. - status tool now runs the live probe (block_height / is_synced / peer_count) when the node is running. - diagnose tool runs the full diagnosis.AllCheckers() suite instead of the previous state-only subset. - All three reuse the same target-resolution helper (mcpResolveTargetFromNode); SSH connection cleanup via defer. .golangci.yml: extended the cmd/* closer-pattern errcheck exclusion to internal/mcp/* (the pattern is identical: target.Target stored in an `interface{ Close() error }` and deferred for cleanup). Verified: every test green, lint exit 0. The cmd surface is unchanged for users; the MCP surface gained three previously-stubbed tools as fully working features.
5 tasks
barbatos2011
pushed a commit
to barbatos2011/tron-deployment
that referenced
this pull request
May 2, 2026
Three actionable findings from the post-implementation review: (#1) MCP `apply` tool description was inherited from the stub days ("currently returns the inputs that would be passed to the CLI; full in-process apply requires extracting cmd/apply.go's RunE..."). That's no longer true — PR tronprotocol#157's `applyTool` calls `apply.Apply` directly. Updated the description to: "Idempotent in-process deploy via internal/apply.Apply (the same pure function the CLI uses)... wait failure leaves the deploy successful but reports ready=false in the result." (#2 false positive) Reviewer flagged `runChild` appending --output json after `rollback <node>` as broken arg order. Verified via real CLI invocation: cobra accepts interspersed flags by default, `trond rollback bogus-node --output json` returns the expected NODE_NOT_FOUND envelope. No fix needed; reviewer concern noted for the record. (tronprotocol#3) `internal/apply/apply.go::detectJDK` used inline atoiSafe / hasPrefix / indexByte helpers "to keep imports trim." That's penny-wise, pound-foolish: atoiSafe disagrees with strconv.Atoi on edge cases (leading whitespace returns 0 instead of parsing past it; negative inputs return 0 instead of the negative number). Replaced with stdlib strings + strconv. The only behavioral change is "more correct on edge cases that are unlikely but possible" — a `java -version` output with leading whitespace would have silently fallen back to JDK 17 before; now it parses correctly. Test additions: - cmd/plan_test.go: TestSimpleHOCONDiff_DetectsLineChanges, EmptyWhenIdentical, HandlesUnequalLength + a smoke test for printDiffSection's three branches. Pure functions, easy wins. - internal/observability/otel_test.go: 8 cases covering schemeOf, stripScheme, parseTimeout (default / override / invalid), parseHeaders (empty / two entries / whitespace-trimmed / malformed-skipped), and TestInit_NoOpWhenEndpointUnset which confirms Init returns a non-nil shutdown function and the default no-op path doesn't error. All tests pass; lint exit 0.
kuny0707
approved these changes
May 2, 2026
kuny0707
pushed a commit
that referenced
this pull request
May 2, 2026
Three follow-up improvements layered on top of the apply extraction (PR #157), each enabled by a piece of that refactor. (5) `trond plan --diff` - Surfaces the line-by-line HOCON content delta alongside the existing field-level changes[] array. Reviewers see WHICH config keys would actually change, not just that hashes drifted. - Sourced from the deployed config persisted under ~/.trond/deployments/<name>/<name>.conf vs the freshly-rendered HOCON; line-by-line via the same simpleDiff shape cmd/config/diff uses. JSON output gains "config_diff": [...] when --diff is set. (6) `trond network upgrade <network>` - Rolling upgrade across every node in a private network. Default order: fullnodes first (one at a time), then witnesses (one at a time) so block production stays up during rollout. --witness-first inverts the order for cases where the operator wants witness ABI first. - Each node is upgrade'd then immediately verify'd; failure halts the rollout. With --auto-rollback, every already-upgraded node gets `trond rollback`'d before exit; without it, the operator is told which nodes need manual revert. - Each per-node step is a re-exec of the trond binary itself for clean lock semantics + independent logging. No shared in-process state across nodes. (7) OpenTelemetry trace export - New internal/observability package with Init() / Tracer() / Shutdown contract. main.go calls Init at start, Shutdown before os.Exit so spans flush. - Default is no-op tracer (zero overhead, no network IO). Opt-in when TROND_OTLP_ENDPOINT is set; supports OTLP-over-HTTP with auto scheme detection (http://... → insecure, https://... → TLS), TROND_OTLP_HEADERS for auth, TROND_OTLP_TIMEOUT for shutdown flush budget (default 5s). - Failure to initialise the exporter falls back to no-op with a stderr warning, so missing collectors never break the user's actual trond invocation. AGENTS.md: documents --diff under Workflow 1, the new `network upgrade` step under Workflow 4, and TROND_OTLP_ENDPOINT in the universal-calling-convention section. Lint exit 0. All existing tests pass; no in-package tests added for network upgrade (subprocess re-exec makes meaningful unit tests require a fake trond binary fixture; deferred until the e2e suite lands).
kuny0707
pushed a commit
that referenced
this pull request
Jun 12, 2026
New `proto-drift` job in .github/workflows/ci.yml that re-runs scripts/gen-dbfork-protos.sh and fails if internal/dbfork/proto/pb/ changes. Catches two regression classes: 1. Upstream .proto edit via git subtree pull without re-running the gen script. Committed Go bindings would silently lag the proto definitions and the engine would marshal against stale schemas. 2. Hand-edit of a *.pb.go file. The files look like ordinary Go and tempt operators to "just tweak" — but they're machine- generated and the next regen clobbers them. The gate uses `arduino/setup-protoc@v3` for protoc + pins protoc-gen-go to v1.36.11 (matching google.golang.org/protobuf in go.mod). Mismatched generator vs runtime versions produce cosmetically-different .pb.go output that would fail the diff for the wrong reason — Task #157 will consolidate the pin into tools.go so there's a single source of truth. proto/README.md: docs the v1.36.11 pin + the new CI gate so future contributors know which version to install + why the diff fails if their version is off. TDD-verified locally: introduced a sentinel comment in Tron.pb.go, confirmed `git diff --exit-code` returns 1; restored, returns 0. Regenerated pb/ with locally-installed v1.36.11 — output is byte-identical to the committed bindings, so CI will start green on the next push.
kuny0707
pushed a commit
that referenced
this pull request
Jun 12, 2026
Replaces the duplicated v1.36.11 pin (CI yaml + proto README + implicit-via-go.mod-runtime) with a single source of truth: the Go 1.24+ `tool` directive in go.mod. Mismatched generator vs runtime versions are now structurally impossible — both the runtime (`require google.golang.org/protobuf v1.36.11`) and the generator (`tool google.golang.org/protobuf/cmd/protoc-gen-go`) resolve from the same go.mod entry. Pieces: - go.mod: `tool google.golang.org/protobuf/cmd/protoc-gen-go` added via `go get -tool`. No version literal duplicated anywhere — `go install tool` reads the pin from here. - .github/workflows/ci.yml: proto-drift job's install step switches from `go install <pkg>@v1.36.11` to `go install tool`. Comment updated to explain the single-source-of-truth design. - internal/dbfork/proto/README.md: tooling-install section drops the hardcoded version; uses `go install tool` for both macOS + Linux. The "if you see drift, your install is off" debugging hint is preserved. - scripts/gen-dbfork-protos.sh: when protoc-gen-go is missing, the error message now suggests the exact install command (`go install tool`) instead of just pointing at the README. TDD-verified locally: `go install tool` installs v1.36.11 (matches go.mod's runtime version). Re-running the gen script produces byte-identical pb/ output → drift check stays green. Test sweep + lint + shellcheck all clean. The CI yaml's pinned dep table is now exactly as long as it needs to be: a Go version, a protoc version (different toolchain entirely), and the actions used. The protoc-gen-go pin moved to where it belongs — alongside its runtime dep in go.mod.
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
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.
Summary
Extracts the deploy + probe primitives out of cmd/apply.go and cmd/status.go into a new internal/apply package. cmd/* becomes a thin presentation layer; MCP tools that previously returned NOT_IMPLEMENTED or state-only subsets now use the same in-process functions.
This unblocks items 2 + 3 from the post-quartet improvement list.
What ships
New package:
internal/applyapply.goApply(ctx, Options) (*Result, error)— pure-function deploy: render → runtime → state save → optional wait. Includes hash-match short-circuit.wait.goWaitForReady(ctx, target, name, port, timeout)— extracted from cmd/apply.go::waitForNodeReadyprobe.goLiveStatus(ctx, target, node)— extracted from cmd/status.go::liveStatusProbe (block_height / is_synced / peer_count via docker-exec curl)apply_test.gocmd/* shrinks
cmd/apply.go::runApplyshrinks from ~220 → ~110 lines. Lock + HUMAN_REQUIRED gate + output formatting stay; deploy phase is one apply.Apply call. Backward-compatible JSON shape preserved (legacystatus/changeskeys still emitted).cmd/status.go::liveStatusProbeshrinks to a 10-line wrapper that resolves the target then hands off.cmd/plan.go's localsha256hexhelper replaced withapply.IntentHashFromBytes.MCP tools that gained real behavior
apply— was returningNOT_IMPLEMENTED_VIA_MCP; now actually deploys viaapply.Apply. Test changed from asserting NOT_IMPLEMENTED to asserting validation works on a bogus path.status— was returning state-only fields; now runsapply.LiveStatuswhen the node isrunning, surfacing block_height / is_synced / peer_count.diagnose— was running a state-only subset; now runs the fulldiagnosis.AllCheckers()suite (sync_progress, peer_count, disk_space, port_listening, memory_usage, version) just liketrond diagnose <name>does.All three reuse
mcpResolveTargetFromNodefor SSH-aware target resolution + defer Close cleanup.Test plan
make build && make test— every existing test still green; new internal/apply tests cover the no-op + validation paths./bin/trond apply --intent examples/nile-fullnode.yaml --dry-run— same shape as before refactor (no shape regression)Backwards compatibility
status/changes/endpointsmap at the cmd boundary.status/diagnosesee additional fields when the node is running.Lint config note
.golangci.yml: extended the cmd/*closer.Closeerrcheck exclusion tointernal/mcp/*since the pattern is identical there (target.Target stored ininterface{ Close() error }, deferred for cleanup).🤖 Generated with Claude Code