docs: add AGENTS.md (AI-contributor guardrails) + AGENTS_USER_EXAMPLE.md

[MichaelLeeHobbs]

Updated per review. #342 (@kryskool): renamed to vendor-neutral AGENTS.md / AGENTS_USER_EXAMPLE.md, fixed CI build-path wording, rebased onto the Gradle main. @pacmano1: verified the Rhino claims against the shipped rhino-1.7.13.jar and the engine source and corrected them (SQL now parameterized, template-literal failure is silent-not-thrown, for...of works, 7 map accessors, $t/$s caveats), and reframed rule 2 from numeric size caps to SRP/single-level-of-abstraction + verification-scaling — size was never the signal.

Closes #342.

Adds two documentation files to help contributors who use AI coding assistants.

AGENTS.md (repo root) — guardrails for AI-assisted work on the engine

Designed to keep AI contributions scoped, reviewable, and from degrading code quality in a production healthcare engine:

• Push back, don't rubber-stamp — for non-trivial work, state the change + any concrete disagreement + a better alternative before editing (calibrated to blast radius, so no "pushback theater" on a typo).
• Anti-sprawl by structure, not size — one concern, judged by SRP / single-level-of-abstraction (not a file/line cap); a large single-concern change is fine when its verification scales with its reach. No drive-by reformatting or renames.
• Preserve behavior & compatibility — public API / serialization / DB / wire-HL7, plus the subtle breaks (charset, timezone/locale, null-vs-empty, HL7 delimiters); golden-output tests for donkey//serialization changes.
• Healthcare security — no PHI logging; no unsafe reflective deserialization (XStream/ObjectInputStream) / XXE, referencing this codebase's documented RCE history.
• Issue-first-and-wait, red-green tests, MPL header copied verbatim from server/license-header.txt, no slop tells.
• Grounded in the real build (Java 17 + Gradle via the wrapper), the module map, and CONTRIBUTING.md. No orchestration/automation skill by design. Advisory — it also notes which rules belong in CI/PR-template as hard gates.

AGENTS_USER_EXAMPLE.md — starter for a user's own channel/template repo

A copy-me AGENTS.md for JavaScript that runs on the Rhino runtime: ES5/Rhino constraints (verified against Rhino 1.7.13), the loop-scoping bug, Java-interop gotchas, engine globals, and code patterns. Points at oie-examples and the docs site. (Could alternatively live in oie-examples — happy to move it as a follow-up.)

Disclosure

Both files were drafted with AI assistance (Claude Code). The content reflects my years of hands-on Mirth/Connect experience and research, and I have reviewed both in full and stand behind them. Feedback and scope changes welcome.

[Copilot]

Pull request overview

Adds repository documentation intended to guide AI-assisted contributions and provide a copyable starter template for users writing OIE channel/template JavaScript.

Changes:

• Adds CLAUDE.md at the repo root with guardrails for AI-assisted work on the engine codebase.
• Adds CLAUDE_USER_EXAMPLE.md as a copy-me starter CLAUDE.md for user-owned channel/template JavaScript repositories.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
CLAUDE.md	Documents AI-contributor guardrails and contribution/build expectations for engine changes.
CLAUDE_USER_EXAMPLE.md	Provides a user-repo template describing Rhino/JS constraints and recommended patterns for deployed scripts.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Test Results

  112 files  ±0    216 suites  ±0   6m 59s ⏱️ + 1m 13s
  654 tests ±0    654 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 308 runs  ±0  1 308 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cef5fd1. ± Comparison against base commit 7e35abe.

[MichaelLeeHobbs]

Thanks for the review — both addressed in 9af1d8c:

1. CI build path — correct, fixed. The -DdisableSigning=true -Dcoverage=true command is the PR build (and what to run locally); on main CI runs the signed build (same target, without those flags). Reworded to say that.

2. Arrow functions — respectfully, this one I'd push back on. Rhino 1.7.13 does support arrow functions; the feature is gated on the Context language version, and OIE ships rhino.languageversion = es6 by default (server/conf/mirth.properties), which puts the bundled Rhino in ES6 mode. So arrow functions (and let/const/destructuring) work out of the box on a stock OIE install. I've kept the line as "supported" and added a note making the ES6-default explicit — and flagging that a server which overrides that setting to an older version should re-verify the borderline features.

[kryskool]

This part must adapt to use the new gradle build

[MichaelLeeHobbs]

Good catch, thanks — fixed in aa5ca2a. I also rebased the branch onto the post-Gradle main (it was still based on the Ant tree).

The build section now points at the wrapper and mirrors what CI actually invokes:

• PRs: ./gradlew build -PdisableSigning=true -Pcoverage=true
• main: ./gradlew build (signed)

Everything else in the section checked out as still accurate against the new layout (.sdkmanrc/JavaFX JDK, and JUnit results still under */build/test-results/**/*.xml), and the other paths the file cites (server/license-header.txt, server/conf/mirth.properties) did not move.

While verifying I pulled two things out of your new CONTRIBUTING.md that seemed worth calling out to an agent specifically, since both fail CI in ways the diff does not explain — the cold-cache dependency-verification-metadata refresh, and the output-parity check on build-logic changes. I kept them to one line each and deferred the detail to CONTRIBUTING.md rather than duplicating it. Happy to drop them if you would rather that guidance live in exactly one place.

[pacmano1]

Strong PR. Before commenting I verified this against current main and against the actual Rhino we ship, and most of it holds up impressively well. The build section is fully accurate (wrapper, .sdkmanrc, the exact CI flags per branch vs PR, the cold-cache verification-metadata trap, the parity guard), the referenced files all exist, the utility-class list matches userutil, and the global-map claims match the source (both stores are ConcurrentHashMap, so the no-null/NPE warning is right). Thanks for grounding it instead of hand-waving.

I ran the Rhino claims against the rhino-1.7.13.jar we ship, at Context.VERSION_ES6 (what rhino.languageversion = es6 maps to in DefaultConfigurationController), at optimization levels -1 (our default), 0, and 9, both at top level and inside a wrapping function to match how the engine executes scripts. Results are identical across all of it. One change I'd insist on, then a set of corrections.

1. The SQL example teaches injection. The "String building" section shows ['SELECT * FROM orders WHERE id = ', id, ' AND status = ', status].join('') labeled "Array join — preferred for SQL". That's string-concatenated SQL presented as house style, in a doc whose whole purpose is to be replicated verbatim by AI assistants writing code for an engine that carries PHI. Your own engine-side AGENTS.md says "be careful with SQL". Please make the example a parameterized query (DatabaseConnectionFactory connection + prepared statement with bound params, or executeCachedQuery with a parameter list) and keep the join() pattern for non-SQL string building. Same example should also drop SELECT *.

2. Two caveats on the helper conventions. $t as shown swallows every exception, and the usage examples wrap method calls (msg.get(...)), not just property chains. A DB error or Java exception inside the callback becomes a silent undefined, which is the exact failure mode this doc exists to prevent. Suggest a caveat: $t emulates ?. for property access only, never around calls with side effects. And $sleep deserves one line noting it blocks a channel thread at full message throughput. (Credit where due: your if (typeof $t === 'undefined') guard pattern verifies correctly on 1.7.13, I checked.)

3. The loop-scoping section is right but incomplete. Your const-in-loop description reproduces exactly (0,0,0 where spec says 0,10,20), it is mozilla/rhino#326, open since 2017. let with an initializer in a loop body works as you say. But there's a second defect your text actively reassures people about ("declarations outside the loop are unaffected"): closures capturing the control variable of for (let i = ...) or for (let k in ...) all see the final value:

var fns = [];
for (let i = 0; i < 3; i++) { fns.push(function() { return i; }); }
// spec: fns[0]()===0, fns[1]()===1, fns[2]()===2
// Rhino 1.7.13: all three return 3

Same for for (let k in obj), every closure gets the last key. No upstream issue covers this as far as I can find (closest umbrella is rhino#939). Suggest adding: never capture a for (let/const ...) control variable in a function created inside the loop, copy it to a body-level let first, which is the pattern your own toJsArray example already uses.

4. Split the rules into durable vs version-dependent, and stamp the version. Some of these are 8-year-old open Ecma-incompatibility bugs (rhino#326, #647, #939, #969 for the const/let family) and safe to state as hard rules. Others are version-scoped: template literals were implemented in Rhino 1.7.14 (Jan 2022 release notes, issue #243), so they're broken on our 1.7.13 and fine one version later. And rhino#1386 (initializer-less let x; in a loop retaining the previous iteration's value) does NOT reproduce on 1.7.13, it's a later regression that would arrive with a Rhino bump. You already have a "version-dependent, verify on your server" bucket, template literals belong there with the precise boundary. Beyond sorting: the runtime section should say "verified on Rhino 1.7.13" inline, because every claim in it is a fact about a specific jar, and the day this repo bumps Rhino for CVEs, several silently flip. I have the probe harness that produced all these numbers and I'd rather commit it somewhere as the regression check than have the doc drift.

5. Template literals: the failure is silent, not a runtime error. On our 1.7.13 backticks evaluate cleanly and skip interpolation:

var mrn = '123456';
var filename = `result_${mrn}.hl7`;
// Expected: result_123456.hl7
// Actual:   result_${mrn}.hl7  (literally, no error thrown)

Nothing errors, the channel stays green, and a File Writer keyed on that value writes every patient's result to one literally-named file. Suggest changing "fails at runtime" to "evaluates without error and emits the raw uninterpolated text". An AI that tests a backtick, sees no exception, and concludes the doc is stale is exactly the failure mode you're guarding against.

6. for...of is in the prohibited list but works. for (var x of array) runs correctly on 1.7.13 at all optimization levels. Under a heading that says "will break at runtime in OIE", a demonstrably false rule undermines the true ones. Move it to supported (keep "prefer forEach/indexed for" as style advice if you like). Opposite nit: spread isn't "minimal/unreliable", it's a hard parse error in both call and array-literal position, feel free to state it as such. The rest of the prohibited list verified exactly as written (default params, class, ?., ?? all parse errors; Promise undefined; Map/Set/Symbol present).

7. Map accessors: two missing, one wrong. The engine defines seven, not five (JavaScriptBuilder.appendMapFunctions): you're missing $co (connectorMap) and $r (responseMap), both two-arg capable. And "one arg gets, two args sets" is wrong for $s: sourceMap is bound via Collections.unmodifiableMap, so $s(key, value) throws UnsupportedOperationException. There's also a plain $('key') that searches responseMap, connectorMap, channelMap, sourceMap, globalChannelMap, globalMap, configurationMap in that order and returns '' (not null/undefined) on a total miss, worth documenting since $('x') == null checks never fire. $cfg does accept a two-arg put, presenting it as get-only is fine advice but say the engine won't stop you, and that runtime writes there don't persist.

8. Drop the numeric caps in rule 2. This is my one real disagreement on the engine-side file. The ≤3-file / ~40-50-line caps, and the prime directive's "the project does not want large multi-file, single-PR agent changes", would have branded the Ant-to-Gradle migration (#326) unwanted, and that was a large agent-assisted PR that's now the foundation of the build. It also contradicts your own build section: a routine dependency bump regenerates gradle/verification-metadata.xml, a generated diff of hundreds of lines, tripping the cap on the most routine maintenance task we have. Size was never the signal, verification is. Suggest replacing the caps with a verification-scaling rule: red-green test for a small fix, golden-output tests for serialization paths, a maintainer-agreed verification plan (like #326's parity harness) for anything sweeping. Keep the parts of rule 2 that do target slop: one concern per PR, no drive-by reformatting or renames.

Housekeeping: your enforcement note says the CI gates "would be a good first contribution", let's file those as actual issues at merge time so they don't stay prose. Happy to take you up on moving AGENTS_USER_EXAMPLE.md to oie-examples as you offered, either way works for me. And the PR body still says the doc is grounded in "Java 17 + Ant server/mirth-build.xml", the file itself is already correct about Gradle, so just refresh the description.

[MichaelLeeHobbs]

This is the review I was hoping for — thanks for running it against the actual jar instead of eyeballing. Nearly all of it is in; details below, and two places where verifying turned up a wrinkle worth your eyes.

1. SQL injection — fixed, and you're right to insist. Split "String building" from a new "SQL — parameterize, never build query strings" section; the example is now a DatabaseConnectionFactory connection + executeCachedQuery(sql, java.util.Arrays.asList(...)) with ? placeholders, and SELECT * is gone. join() stays only for non-SQL strings.

2. $t / $sleep — done, with a nuance I want to keep. $sleep now notes it blocks the channel thread. On $t: the exception-swallowing is by design — it's what makes the $t(() => risky()) || default idiom work, and it stands in for ?. on property navigation. But you're dead right that it's broader than real optional chaining (?. propagates a thrown error; $t eats it), so I documented exactly that: use it for navigation and best-effort reads, never around a call whose failure must surface (DB write, ACK send) — that's the silent-undefined trap. Reads as "here's the footgun," not "don't use it."

3. Loop-scoping closure capture — added. New example showing all closures made inside for (let i…) capture the same binding (return the final value on 1.7.13), with the copy-to-body-level-let fix. Tied it back to why toJsArray already does that.

4. Split durable vs version-dependent + version stamp — done, and yes please on the harness. The runtime section now says "verified against Rhino 1.7.13" and flags version-scoped items inline (template literals as 1.7.14+). Committing your probe harness somewhere as the regression check is exactly right — every claim in that section is a fact about one jar, and I'd much rather it be executable than prose that silently rots on a Rhino bump. Where do you want it to live?

5. Template literals silent, not thrown — fixed. Reworded to "evaluates without error and skips interpolation, emitting the raw ${...} text," with the File-Writer-writes-every-message-to-one-file consequence spelled out, plus the 1.7.14 boundary.

6. for...of and spread — both fixed. for...of moved out of "prohibited" (it runs) into the supported list as a style preference — I kept it discouraged, but on throughput/consistency grounds rather than a false "it breaks" claim. Spread is now stated as the hard parse error it is.

7. Map accessors — mostly adopted, one thing to check with you. Confirmed against appendMapFunctions: it's seven, $co and $r added; $('key') documented as searching all maps and returning '' (not null) on a miss; $cfg two-arg put documented as succeeding-but-not-persisting. On $s specifically I hit a wrinkle before writing "throws": the scope-binding path for transformers/filters (JavaScriptScopeUtil.addConnectorMessage) wraps the raw source map — SourceMap(ImmutableConnectorMessage.getSourceMap()) → ConnectorMessage.getSourceMap() → MapContent.getMap(), no Collections.unmodifiableMap — whereas the batch adaptors explicitly wrap it unmodifiable. So by static reading $s(k, v) should throw in batch scripts but not in a transformer. You saw a throw empirically — which script context was your harness in? I've documented $s as get-only regardless (relying on the write is unsafe across contexts), but I'd like the doc to name the exact boundary rather than overstate it, and your repro would settle it.

8. Dropping the numeric caps — agreed, and thanks for the push. You're right that the ≤3-file / ~40-50-line caps were a proxy that misfires both ways (they'd have branded the Gradle migration unwanted; a routine verification-metadata.xml regen trips them; a tiny God-function PR sails through). I split what they were conflating: reach of a change vs structure of the code. Rule 2 now judges by one-concern + SRP/single-level-of-abstraction (with orchestration/init and low-complexity sequences called out as legitimate long-function exceptions), and defers size-of-reach to verification scaling (rule 5). The prime directive now says a large single-concern change is fine with a proportional verification plan, and names the migration as the example — "unscoped, unverified sprawl is the enemy; size is not." Dropped the explicit LOC numbers entirely; SRP/SLA carries it.

Housekeeping:

• Refreshing the PR description (the stale "Java 17 + Ant" line) — the files were already correct, just the body lagged.
• Happy to move AGENTS_USER_EXAMPLE.md to oie-examples — I'll do that as a follow-up so this PR stays engine-only, unless you'd rather it ride along here.
• Will file the enforcement-note CI gates (linked-issue check, license-header check, no-dep/build-change-without-label, whitespace/import-only-hunk rejection, emoji/AI-tell grep) as real issues at merge so they don't stay prose.

Everything above is pushed. The one open question is the $s context in #7.

[MichaelLeeHobbs]

Strong agree, and it's the missing half of what rule 2 was reaching for. We reframed rule 2 so verification scales with a change's reach — but that only makes the author verify. Your point is the reviewer-facing half: the PR has to make that verification reproducible by someone who didn't write it. Green CI is the dashboard light going off; it isn't turning the key.

The car analogy earns its place — I'm making it canon in rule 5, which currently stops at "a regression test fails-before/passes-after." It'll now also require a How to verify section: the concrete before/after a reviewer can run — failing input + wrong output before, correct output after — and for anything a unit test can't show (wire bytes, a deployed channel, a REST path), the exact steps and the observable result.

One thing that makes this a no-brainer: CONTRIBUTING already demands "steps to reproduce / expected vs. actual" — but only from bug reports. PRs are asked for "a brief description." So we hold the report of a bug to a higher bar than the fix for it. This is just closing that gap.

The real enforcement is a PR template (.github/PULL_REQUEST_TEMPLATE.md) with a required "How to verify" field — there isn't one today. I'd keep it out of this PR (adding a template to a docs PR is its own scope creep), but I'm happy to open it as a small separate PR if you want it, or leave it to you since it's a process change you'd own. Either way.

And yes — exactly the shape for your SQL injection fix: not just "parameterized now," but here's the injectable input and what it did before, here's the same input bound as a parameter and the query it actually runs after. Start the car. If a worked example helps, the #344/#351 write-up is one — same message in, literal undefined on the wire before, intact HL7 after, with the steps to reproduce both.