Repository navigation
Conversation
Ranked findings (security, correctness, design, tests/tooling) with file:line references and a suggested order of work. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…ents and dead code - go mod tidy: the five direct dependencies were marked // indirect and go.sum lacked entries tidy expects. - .gitignore: data/ and out/ were root-anchored, so `make run` (which writes examples/data and examples/out) dirtied the tree. - Remove "phase N" comments left over from the original build plan. - mllp-listener: the empty if on a framing error is now a warning log. - Remove unused astm1381 soh constant, Duration.MarshalYAML, and the tui max helper that shadowed the builtin. - Document astm-sender maxRetries as an attempt count. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
- errorlint: compare io.EOF with errors.Is; wrap errors with %w in http-sender. - gosec: TLS 1.2 minimum for the http-sender CA pool; render web UI badges through html/template instead of Sprintf into template.HTML; ReadHeaderTimeout on the API server. - noctx: listeners bind through net.ListenConfig with the run context; store migrations use the Context variants. - staticcheck/unparam/unused: pathID no longer takes a constant key, drop the unused messagesQuery type, do not call t.Fatal from the SSE test's feeder goroutine, remove an empty branch in a test. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
CI runs go mod tidy -diff, golangci-lint, and make check (gofmt, vet, race tests) on pushes to main and on pull requests. make lint now includes tidy-check and golangci so the local gate matches CI. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…s-site requests The API is the daemon's full-control surface (channel lifecycle, message replay, script editing) and it served every route anonymously on all interfaces. Now: - daemon.yaml gains an auth block: auth.token is accepted as a Bearer token and as the basic-auth password with any user name, so the same secret works for API clients and the browser prompt; basicUser and basicPassword add a dedicated login; auth.disabled opts out. - The default listen address is 127.0.0.1:8420, and binding a non-loopback address without credentials is a config error. - One middleware wraps the whole route table, static assets included, with constant-time credential comparison. - State-changing requests carrying a browser Origin must match the request Host, and Sec-Fetch-Site must be same-origin. Basic auth is ambient, so without this a page on any origin could drive the daemon. Tests cover each credential form, every route class, the disabled switch, the cross-site guard, and the config policy table. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
ScriptsRoot is the channels directory, so the old prefix check let a client PUT any file under it, channel YAML included, and have the reload action apply it. A path now qualifies only when it is a .js file that a loaded channel references and it still resolves under the root after symlinks are followed. The root itself, traversal, unreferenced files, and symlinks pointing outside are rejected with one generic error; the reason is logged rather than echoed. The body is read through MaxBytesReader with io.ReadAll: an oversized upload answers 413, a truncated one 400, and neither reaches disk. The file is replaced via temp file and rename so the hot-reload watcher can never compile a partial write. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…ric internal errors - API server: ReadTimeout, IdleTimeout, MaxHeaderBytes alongside the existing ReadHeaderTimeout. WriteTimeout would cut SSE streams, so the auth middleware sets a per-response write deadline and exempts the event endpoints. - http-listener: WriteTimeout sized to cover the destination-mode hold, a configurable idleTimeout (default 60s), MaxHeaderBytes. - SSE: concurrent streams are capped (MaxEventStreams, default 64) with 503 + Retry-After beyond that. - limit is validated and clamped to 500 on the message and DLQ lists; before_id is validated; bad values answer 400 instead of being silently ignored. - Internal errors are logged and answered with a generic message so filesystem paths and SQL detail stay out of response bodies. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
A script could write an absolute URL into meta['http.path'] and ResolveReference would send the whole request there. The path is now rejected as a permanent error when it carries a scheme, host, userinfo, or a protocol-relative prefix; the destination host stays the operator's YAML decision. Successful response bodies are drained through a 1 MiB limit rather than read to completion. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
Both parsers recurse once per nesting level. A payload consisting of nothing but open brackets or open tags, well under the 10 MiB default body limit, overflowed the goroutine stack; that is a fatal runtime error, not a panic, so nothing could recover the process. Nesting past MaxDepth is now a parse error, which routes the message to the invalid message channel like any other malformed input. Tests cover the limit boundary and the megabyte-of-brackets shape of the original crash. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
Pause held c.mu while calling Source.Stop. The MLLP and ASTM listeners stop by waiting for their connection goroutines, and a connection that had just read a frame was inside deliver, whose first action is to lock c.mu. Each side waited on the other and the channel was wedged for good. Lifecycle operations now serialize on their own mutex, which is held across adapter calls; c.mu guards only the status fields and is never held while calling into an adapter. A message that arrives during the pause transition is accepted and processed, since the transport has already read it and must answer. The regression test hangs on the old code and passes now. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…nored Every Recorder call after Record discarded its error. The worst case: SetDestinationState(QUEUED, payload) failed, Enqueue succeeded, and the worker read the queue row, found no payload behind it, and sent empty bytes. Now: - A failed SetState/SetTransformed routes the message to the invalid message channel and answers AE with a persistence-failure text, so a destination-ACK sender retries rather than treating it as accepted. - A channel-level serialize failure is an error, not a message marked TRANSFORMED with no payload stored. - The QUEUED record is checked before Enqueue; if it fails the delivery is recorded as ERROR and never queued. - Store failures on outcome records (SENT, FILTERED, ERROR) are logged with full context rather than dropped. - Head reports a queue row with no recorded payload as Orphaned and the worker dead-letters it instead of sending nothing. Tests inject failures at each seam and assert nothing is sent or enqueued. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
deliver spawned a goroutine per message, all serializing on a mutex. A fast MLLP sender in immediate-ACK mode got its ACK as soon as Record returned and kept sending, so goroutines accumulated without limit. Each channel now runs a single pipeline goroutine draining a buffered channel of recorded messages (maxPending, default 256). When the buffer is full the source adapter blocks inside deliver, which delays its transport ACK: a burst becomes backpressure on the sender. Stop stops the source, refuses new intake, waits for handoffs in flight, closes the buffer, and lets the pipeline drain it before adapters close, so nothing accepted is lost. Inject shares the same admission path and works on a paused channel (replay bypasses the source). This also removes the wg.Add/Wait race between Inject and Stop. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…urce.* Metadata keys are a protocol between adapters, scripts, the queue, and the engine, none of which import each other, and they were string literals scattered across six packages. internal/meta now defines them. The http-listener stamped http.method and http.path on every inbound message, the exact keys http-sender reads as per-message routing overrides, so an http-listener to http-sender channel silently replayed the inbound request's path and method against the outbound base URL. Inbound context now lives under source.http.* (method, path, query parameters, content type); the outbound http.* keys remain script-set only. A listener test asserts every inbound key is namespaced and a sender test asserts inbound context does not route. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…race-free file-reader moved a file to the error directory whenever deliver returned an error, but that error means "not accepted right now" (channel paused, store busy), so a transient refusal became permanent loss. The file now stays in place and is offered again next poll. The reader gains ackMode like the network sources: destination mode waits for the pipeline outcome and moves a rejected file to the error directory, an accepted one to processed. Rename errors are no longer discarded: a cross-device move falls back to copy-and-remove, and a file that still cannot be moved is remembered so it is not redelivered every poll. The collision-suffix loop spun forever when the target directory was not a directory; it now stops on any Stat error. file-writer fsyncs before publishing and publishes with os.Link, which fails on an existing name instead of replacing a file another channel wrote between the existence check and the rename. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…explicit shutdown status, O(1) node depth - adapter.Duration rejects bare numbers: holdTimeout: 30 used to mean 30ns and passed every zero check. Negative durations are rejected too. - http-listener: the handoff to the engine runs under a non-cancellable context so Stop cannot fail a Record mid-insert; a request held for the pipeline outcome now answers 202 with the message id on Stop instead of an implicit empty 200; a client that disconnects releases its handler. - mllp-sender gains a write deadline (writeTimeout, default 10s), and both TCP senders honour context cancellation by expiring the connection deadline, so shutdown no longer waits on a stalled peer. - Delimited trees record each node's structural level in Kind, so Value finds it in O(1) instead of searching from the root; getAll over 4000 OBX segments drops from about a second to a millisecond. Golden trees regenerated. - response.setAck from a destination-level script reaches the source ACK; it was lost with the per-destination copy. - Script logger bindings use the injected logger. - Retention pruning is nudged onto its own goroutine instead of running a DELETE on the pipeline's write path every 100 inserts. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…L namespace/charset/whitespace fixes
Delimited formats (HL7 v2, ASTM):
- Set splits its value on the separators below the addressed level the
way the parser splits wire text, so set('PID-5', 'DOE^JOHN') builds
components and set(get(x)) round-trips. Separators at or above the
level stay literal and are escaped on the wire. This is the Mirth
behaviour scripts expect; before, every such write produced \S\.
- Writing below MSH-1/MSH-2 (H-1/H-2) is rejected instead of corrupting
the header.
JSON:
- Set never silently replaces a populated container: a key step on a
non-empty array descends into element 0 (what get reads), an index
step on a non-empty object is an error.
- Bracketed keys use one escape grammar in both directions: Flatten
emits JSON escapes and Resolve now decodes them, so every flattened
path resolves.
- Number leaves are validated against the JSON number grammar rather
than json.Valid, which accepted any JSON value.
XML:
- xml:lang and xml:space serialize again (the reserved xml prefix is
mapped back from its namespace URI).
- ISO-8859-1 and Windows-1252 declarations are transcoded and the
declaration rewritten to UTF-8, which is what Serialize emits.
- Whitespace-only simple content is a value (<a> </a> keeps its space);
only formatting between child elements is dropped.
- Set with a first step naming anything but the document element fails
at Set instead of appending a second root that Serialize rejects.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…channels
internal/testutil replaces six copies of the same setup (temp store,
script engine, engine, channel from YAML, poll the store) and three
differently named wait helpers with one fixture: NewFixture,
StartChannelYAML, StartExample, ListenAddr, Eventually, WaitEvent,
WaitMessageState, WaitForDestinationState, Getter, AckingReceiver.
The engine's end-to-end tests move to package engine_test so they can
import it, and adapter.Addresser replaces the concrete listener type
assertions.
StartExample loads the real examples/channels/*.yaml with only listen
and peer addresses rewritten, so drift between the examples and the
tests is now caught. It caught one: astm-to-mllp.yaml sent results to
adt-to-csv's listener, whose filter drops anything but ADT, so under
make run every bridged result was FILTERED. It now points at
mllp-to-astm's listener as the README describes.
Two sleep-based negative assertions ("no output appeared for the
filtered message") now wait for the message's FILTERED event instead,
which marks the end of its pipeline.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
… astm become declarations hl7v2 and astm were about 85% the same code: header delimiter extraction, tree delimiter recovery, parse, serialize, path parsing, escape decoding and encoding, and all six DataType methods, each copied with a different segment-name length and delimiter order. delimited.Format now implements format.DataType from a Spec that names the genuine differences: the name pattern and length, which segments are headers, the default delimiters, the order of the encoding characters in the header (HL7 "^~\&" is component/repetition/escape/subcomponent, ASTM "\^&" is repetition/component/escape), and the escape-sequence table. The path regexp is derived from the spec, so the subcomponent level exists exactly when the format has one. hl7v2 and astm shrink to their Spec plus DataType delegation; the hl7v2 escape file is gone. Every existing format, script, and adapter test passes unchanged. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
The MLLP and ASTM listeners carried byte-identical lifecycle code (bind, accept loop, per-connection goroutine with AfterFunc close, Stop with WaitGroup) and the two senders the same lazy-connect, drop-on-error, deadline-merging client. internal/adapter/tcp now holds a Server and a Client used by both; the protocol handling stays in each adapter. AckMode, its validation, the accepting-code test (AA/CA), and the destination-mode hold (AwaitDecision with Decided/TimedOut/Canceled outcomes) move to the adapter package. The four inbound adapters (MLLP, ASTM, HTTP, file) share them, which also settles two inconsistencies: the ASTM listener now accepts CA like the others, and both listeners gain a configurable writeTimeout (default 10s) where one had a hard-coded 5s and the other none. The MLLP listener gains an optional idleTimeout. Listener handoffs run under a non-cancellable context so Stop cannot fail a Record mid-insert. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…he interface
Two optional interfaces papered over gaps in format.DataType.
TypedSetter existed because Set took a string while JSON needs to keep
JS numbers and booleans typed; SegmentJoiner existed because the
script engine built segment-relative paths with an HL7-shaped default
join ("SEG-rel") that XML overrode. The default was wrong for the
formats that did not override it: on a CSV row handle row.get('3')
errored with an invalid path, and on a JSON object handle
obj.get('family') silently returned nothing.
Set now takes a Go scalar (string, bool, nil, numeric) for every
format; JSON keeps the type and the text formats store format.String
of it. ResolveFrom and SetFrom join the interface and each format
defines its own relative dialect: field paths on an HL7/ASTM segment,
a column number on a CSV row, a key path on a JSON object or element,
element steps on an XML element. The script engine's segment handles
use them directly. A cross-format test drives segment handles through
CSV, JSON, XML, and HL7 scripts.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…rors; surfaced UI errors The channel list with counts, the script reference list, the message stage list, and the lifecycle action switch were each written two or three times across the JSON handlers, the HTML handlers, and the TUI. They now live in engine: ChannelSummaries/ChannelSummary (with Received/Sent/Errors/Filtered/Queued accessors), ScriptRefs, Stages, and Lifecycle. ErrUnknownChannel and ErrUnknownAction replace string matching on error text for status codes; script.ErrNotLoaded likewise. Web UI: templates render into a buffer so a template error is a clean 500 instead of a truncated 200; a failed channel action or requeue is shown in the refreshed table instead of only logged; store failures no longer render as "no messages"; the message page distinguishes not found from a store error; the DLQ page checks the channel exists; the message list paginates with an "older messages" link. The TUI reads the same summaries and stages instead of recomputing them. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…mands
waggle tui booted a second full engine on the same database, channel
directory, and adapters as the daemon, so running it alongside the
daemon double-processed file inputs and fought for listen ports. It now
observes a running daemon through HTTPBackend: the JSON API for
channels, messages, trees, and diffs, and the SSE stream for events
(reconnecting with backoff and delivering a Resync after a gap). The
address and token default to the daemon config; -addr and -token
override them. GET /api/channels/{id} is added for single-row refresh.
The model no longer does backend I/O inside Update: every load is a
tea.Cmd that returns a typed message, stale results for a view the
operator has left are dropped, and a slow daemon stalls a command
rather than the screen. Tests drive the commands to completion
synchronously; an integration test runs the backend against a real
engine behind a real API server.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…; report failed starts Validate filled in defaults (name, destination dataType) as a side effect; Normalize now does that and Validate is pure. Adapter types were only checked when the engine built the channel while data types were checked at load; both are now checked at load against the registries, so a typo in a channel file fails with the list of registered types before anything starts. The daemon logs how many channels failed to start instead of discarding the errors. With the TUI attached over HTTP, the daemon is the only place an engine is bootstrapped, so there is nothing left to extract. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
… logged prune errors - Reads (message lists, details, counts, queue head) go through a separate read-only connection pool; writes keep their single serialized connection. WAL mode lets the two run concurrently, so UI queries no longer queue behind pipeline inserts. A test holds a write transaction open and shows a read completing alongside it. - Each destination queue has a wake channel that Enqueue and Requeue signal; workers block on it and only fall back to polling (now 1s) for retry deadlines. With N destinations that removes 4N idle queries per second. - A unique index on destination_queue(destination_id, message_id) makes the one-pending-delivery invariant the database's. - Retention prune errors are logged instead of discarded. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
…n fuzzing; reject unusable delimiters Flakiness: - The hot-reload test polls for the recorded compile error instead of sleeping past the watcher interval. - The ASTM duplicate-frame test proves the count is final by sending a second session rather than sleeping; the http-listener stop test waits for the handoff signal instead of 100ms; handler-goroutine results are passed over channels or atomics rather than shared variables. - Closed-port tests use an address that was just released instead of port 1. - Table-driven error cases run as named subtests in sorted order; a no-op backoff assertion now checks the cap. - Golden trees live in each format's own testdata. Coverage, each a behaviour the README promised without a test: a paused channel keeps draining its queue; reload applies rewritten YAML and refuses a changed id; enabled: false stays stopped; a queued delivery survives an engine restart; destination-ACK with a queued destination answers before delivery; the insert-count retention trigger; maxAttempts -1 retrying past many failures; SSE per-channel filtering and resync pass-through; Stop/Start restart of every inbound adapter; MLLP framing violations and oversize frames dropping the connection. The hl7v2 and astm fuzz targets now require the canonical form to be a fixed point (same tree, same bytes). That found two inputs whose delimiters cannot round-trip: duplicate delimiter roles, and digit delimiters that appear inside \X0F\-style escape bodies. Both specs forbid alphanumeric delimiters; the parser now rejects duplicate, alphanumeric, and terminator delimiters at parse and serialize time. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
The review now says up front that its findings have been implemented on this branch and that its line references describe the original commit. README's Development section lists make lint, what it runs, and that it needs golangci-lint installed. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
langhorst
merged commit Sep 15, 2026
88c063b
into
claude/go-integration-platform-plan-amhaw5
1 check passed
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.
Adds
docs/CODE_REVIEW.md(a principal-engineer review of the repository atf2d084d) and implements every finding in it, one commit per finding group, in the order the review recommends.25 commits, 93 files changed. Each commit passed
gofmt,go vet,golangci-lint run, andgo test -race ./...before it was made.Security (P0)
127.0.0.1:8420as the default listen address, and a hard config error when a non-loopback address is used without auth. Cross-site requests are rejected viaSec-Fetch-Site/Origin, which also closes the DNS-rebinding path to the reload endpoint.PUT /api/scriptswas an arbitrary file write under the channels directory. It now writes only.jsfiles actually referenced by loaded channels, resolves symlinks, writes atomically, and bounds the request body.http.pathmetadata override could carry a scheme and host and redirect a send to an arbitrary server; it must now be a path.Correctness (P1)
recover()cannot catch. Both parsers now cap container nesting at 512 levels.Recordercall on the hot path discarded its error, producing queued deliveries with empty payloads. Store failures now fail the message.Source.Stop/Start; lifecycle and state locks are separate.maxPending, default 256) replaces a goroutine per inbound message, giving the source real backpressure.source.*.Setsplits values into components and repetitions instead of writing them literally, JSON containers with existing content are guarded, and XML namespace prefixes, charset decoding, and whitespace-only text are handled correctly. Adapter durations now require units, sends are cancellable, and node depth is O(1).Design (P2)
delimitedformat parameterized by aSpec.tcp.Server/tcp.Client, andAckModeis defined once.format.DataTypetakes typed values and exposes segment-relative resolution.Tests and tooling (P3)
testutilharness replaces six ad-hoc end-to-end setups; tests now run the shipped example channels.Delims.Validatenow rejects duplicate, alphanumeric, and control-character delimiters..golangci.yml, and amake linttarget (gofmt + vet + tidy check + golangci-lint) wired intomake check.Verification
Full suite passes three consecutive times under
-race; lint clean; 15s of fuzzing clean for both hl7v2 and astm.Note for deployers
The daemon now refuses to start on a non-loopback address without auth configured, and
examples/daemon.yamlships a placeholder token. Anyone deploying from the examples needs to set a real one.Base is
claude/go-integration-platform-plan-amhaw5, the repository's current default branch.🤖 Generated with Claude Code
https://claude.ai/code/session_01CDAe95EyukJTfQD3SePK6U
Generated by Claude Code