feat: deployment jobs argument, auto opTokens, and fail-loud multi-op guard - #378
feat: deployment jobs argument, auto opTokens, and fail-loud multi-op guard#378kingpanther13 wants to merge 95 commits into
Conversation
…ecorded untokened writes Ports the issue-376 engine as a deployment ARGUMENT (no new tools): both native-app write tools route args.deployment to one shared job engine -- op create/resume/commit/cancel (confirm-gated) + op status as the pure read mode. Jobs checkpoint to hub storage after every op and advance on-hub via the scheduler with no client attached; staging auto-validates before ready_for_commit; cancel deletes only job-created apps. Also closes the token-less recovery hole: every untokened WRITE now gets a server-assigned auto- opToken (recorded + result buffered exactly like a client token, token returned on the response), and hub_get_info(includeRecentOps=true) lists the op journal so a client that lost a response can find what ran and replay it token-only instead of blind-retrying. slow_ops guide states the token is the assistant's to invent and record -- never the user's.
…first token contract Untokened writes now get full duplicate protection without any client setup: an identical call (canonical-args fingerprint) arriving while its twin's record is still running is refused with the in-flight token to poll, and a maxConcurrentWrites cap (advanced setting, default 2, 1=serial, 0=off, also settable via hub_update_mcp_settings) refuses writes past the concurrency limit, naming the in-flight ops. Both checks age-cap at 10 minutes so a crash-stranded record cannot wedge writes. Client-tokened calls keep their own dedup and skip the fingerprint check (an explicit new token = intentional re-run). slow_ops guide and the opToken descriptions now present the auto-record as the primary mechanism (client tokens = optional verbatim-retry extra) and state record lifetimes. Every write tool's outputSchema declares the returned auto token. Dispatch specs + a live-hub e2e (auto token -> journal -> token-only replay) cover the new surface. Implemented by codex-cli; reviewed, line-ending-normalized, and verified.
…ror) A bare assignment cannot continue onto a leading-&& line -- the hub's parser rejected the class at line 1640 (live deploy caught it; sandbox lint does not parse). Wrapped in parens like every other multi-line boolean in the file.
…AT findings Two wire-verified bugs from the zero-context BAT runs: - hub_set_rule EDIT with multiple operation families silently ran only the first while reporting clean success. Now rejected fail-loud BEFORE any write, naming the ops and steering to patches; the documented addTriggers+addActions bulk pair stays allowed. settingsApplied deduped. - A completed op's journal record could stick at 'running' when the prune's whole-map rewrite clobbered the per-entry completion write. _opTokenComplete/_opTokenRelease now re-read after writing and re-write the terminal state once if reverted. Plus the queued findings: hub_get_rule_health now returns live eventSubscriptionCount/scheduledJobCount (the readback T311's agents proved unreachable); hub_list_rules reports the stopped sub-state and strips the '(Stopped)' suffix; nonexistent-rule edits lead with 'No rule/app with id N exists'; addTrigger names the canonical 'Certain Time (and optional date)' picker string; pvTF/pR inverted-storage quirk documented; hub_list_files gains the name filter its callers expected; TOOL_GUIDE slow-ops synced with the served guide; BAT suites get the zero-context runner contract, T311/T432/T433 rewrites matching the real tool surface, and T661's fail-loud pass-path. Spock coverage for the guard, the allowed bulk pair, and the completion re-write. Implemented by codex-cli (gpt-5.6-sol); reviewed and verified.
…e it) Live verification caught the stopped-status fix not working through hub_list_rules: the cheap RMUtils/appsList sources never carry the '(Stopped)' decoration on this firmware -- it only surfaces on per-app pages. hub_get_rule_health now derives an authoritative boolean from the already-fetched config label (and strips the raw HTML span + decoration from the returned label, which previously leaked). List/docs/BAT wording corrected: list-level 'stopped' is best-effort, health is authoritative; a stopped rule's statusJson omits its subscription list, so eventSubscriptionCount reads null (not 0) while stopped -- documented and T311 reworded accordingly.
…ototype pointers in engine text
The deployment argument had no way to remove a terminal job record (they
persisted until the 8-job cap forced a lazy evict). op=delete removes a
completed/cancelled record only -- apps and backups untouched, rollback
handles surfaced one last time. Engine error/note text still pointed at
the prototype-era hub_get_deployment tool and operation= key; all sites
now steer to the real surface (deployment={op:...}).
Adds the missing test coverage for the whole argument: ToolDeploymentJobsSpec
(routing, status read, create fail-before-persist, delete gating), a
deployment lifecycle e2e scenario (native_apps group + e2e_scope map entry),
and BAT T465/T466.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used📓 Path-based instructions (4)**/*.groovy📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/test/groovy/**/*.groovy📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{groovy,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-08-08T03:29:24.112ZApplied to files:
📚 Learning: 2026-08-10T17:02:34.806ZApplied to files:
📚 Learning: 2026-08-09T12:53:41.770ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe PR adds durable multi-application deployment jobs, automatic operation-token replay, concurrent-write limits, stopped-rule health reporting, file filtering, expanded tool schemas, and automated coverage. ChangesServer behavior and deployment
Tool contracts and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…to the branch's new behavior The reachability guard caught a real gap: the deployment_jobs guide section was served but missing from hub_get_tool_guide's section enum. The remaining failures were stale pins on intended changes: dispatch tests now see the auto-token op-result buffer upload (filtered at the capture stubs), the hub_list_rules status enum gained 'stopped', hub_set_native_app's schema gained 'deployment', and the nonexistent-rule backup error leads with the 404 fact instead of backup mechanics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (13)
src/test/groovy/server/ToolManageFilesSpec.groovy (1)
57-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend
hub_list_filesfilter coverage.
- Pass
filterthroughmcpDriver.callToolin both gateway modes.- Add a case-insensitive filter assertion for the HTML fallback.
- Add a cursor-pagination case that proves filtering occurs before pagination.
🤖 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 `@src/test/groovy/server/ToolManageFilesSpec.groovy` around lines 57 - 74, Extend the hub_list_files tests around the existing “hub_list_files filters file names by case-insensitive substring” case: invoke the tool through mcpDriver.callTool with filter in both gateway modes, add an HTML-fallback assertion for case-insensitive filtering, and add cursor-pagination coverage proving the filter is applied before pagination.Source: Coding guidelines
libraries/mcp-deploy-jobs-lib.groovy (3)
407-407: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
jobIdunique against same-millisecond creates.
"dj-${now()}"collides if two create calls land in the same millisecond._deploySaveJobthen overwrites the first job record throughupdateMapValue, and the first job'screatedAppIdsare lost, so its apps can no longer be rolled back byop='cancel'. Append a short random or counter suffix.♻️ Proposed change
- def jobId = "dj-${now()}".toString() + def jobId = "dj-${now()}-${(int) (Math.random() * 10000)}".toString()🤖 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 `@libraries/mcp-deploy-jobs-lib.groovy` at line 407, Update the jobId construction in _deploySaveJob to append a short uniqueness suffix to the current timestamp, using an existing random or counter utility where available. Preserve the “dj-” prefix and timestamp while ensuring same-millisecond create calls produce distinct IDs.
290-300: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the mutated
statusListback to the job before saving on the gated path.The
addActionsgate mutatesstatusList[idx]at line 294 and calls_deploySaveJob(job)at line 298, but it never assignsjob.opStatus/job.commitStatusthe way line 338 does. The code works today only because((staging ? job.opStatus : job.commitStatus) ?: []) as Listreturns the same list instance. Ifjob.opStatusis ever absent or is deserialized into a non-Listcollection, the elvis branch builds a detached list and thefailed/interruptedmarks are lost on save. The adopt branch at lines 279-286 has the same gap.Assign the list back before each
_deploySaveJobcall, as line 338 does.🤖 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 `@libraries/mcp-deploy-jobs-lib.groovy` around lines 290 - 300, Assign the mutated statusList back to the appropriate job.opStatus or job.commitStatus field before calling _deploySaveJob(job) in the addActions gated path. Apply the same assignment in the adopt branch, preserving the staging-based field selection used by the existing status-list logic.
356-366: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the worker re-arm when a slice throws repeatedly.
If
_deployRunSlicethrows before it can mark the jobfailed, the catch at line 358 logs, the finally clears the lease, and line 366 re-arms the worker because the job is still in an active phase. The next run repeats the same throw. The job then re-schedules every 15 seconds forever and no phase transition ends it.Count consecutive slice exceptions on the job record. When the count passes a small limit, set
phase = "failed"with the last error so the operator sees it and the re-arm stops.🤖 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 `@libraries/mcp-deploy-jobs-lib.groovy` around lines 356 - 366, Update the worker exception handling around _deployRunSlice to track consecutive slice exceptions on the job record, incrementing the count and retaining the latest error. When the count exceeds a small bounded retry limit, set the job phase to "failed" with the last error so the existing stillActive check stops re-arming; reset the consecutive-exception count after a successful slice.libraries/mcp-native-rules-lib.groovy (2)
7757-7758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
unique(false)instead of a manual dedup loop.
uniqueAppliedbuilds a deduplicated list with a manualcontainscheck insideeach.List.unique(false)does the same in one call, preserving insertion order without mutatingapplied.♻️ Proposed simplification
- def uniqueApplied = [] - applied.each { key -> if (!uniqueApplied.contains(key)) uniqueApplied << key } + def uniqueApplied = applied.unique(false)Also applies to: 7768-7768
🤖 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 `@libraries/mcp-native-rules-lib.groovy` around lines 7757 - 7758, Replace the manual deduplication loops for uniqueApplied, including the corresponding logic at the second occurrence, with List.unique(false) on applied. Preserve insertion order and avoid mutating the original applied list.
152-161: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
deploymentis undocumented in the flat self-gateway tool.
toolSetRulenow dispatchesargs.deploymentbefore theoperationenvelope is even inspected (line 9426-9431), so a flat-mode caller (useGateways=false) can technically senddeployment. But_setRuleFlatTool()— the description andinputSchemaserved in flat mode — never mentionsdeployment, and_setRuleOperations()does not include it as a selectable operation. An AI client relying on the flat tool's self-description has no way to discover this capability exists.Add a brief mention of
deploymentto_setRuleFlatTool()'s description (and consider adding the property to itsinputSchema) so flat-mode callers can discover the feature the same way gateway-mode callers can from this fat schema.Also applies to: 283-293
🤖 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 `@libraries/mcp-native-rules-lib.groovy` around lines 152 - 161, Update _setRuleFlatTool() to document the deployment capability in its flat-mode description and inputSchema, matching the existing deployment definition and behavior exposed by the gateway schema. Ensure flat-mode callers can discover and submit deployment without changing dispatch or operation handling.tests/BAT-v2.md (1)
4627-4627: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a dedicated scenario for the mixed bare-argument rejection.
T661 only asks for several changes in one batch. An agent can use
patchesdirectly and pass without exercising the new fail-loud guard. Keep T661 focused onin_progressresume behavior. Add a separate goal-framed BAT case that requires the rejection, followed by a successfulpatchesretry.🤖 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 `@tests/BAT-v2.md` at line 4627, Add a separate goal-framed BAT scenario for mixed bare-argument batches that verifies the call refuses fail-loud without partial application, then retries the same changes through patches successfully. Remove this rejection requirement from T661 so it remains focused solely on in_progress resume behavior.hubitat-mcp-server.groovy (3)
1573-1591: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAuto-token generation uses a 16-bit random suffix.
Two writes started in the same millisecond can collide with probability 1/65536. A collision makes the second
_opTokenMarkoverwrite the first record, so the first operation's poll replays the wrong buffered result. The concurrency cap makes this rare, but a wider suffix removes the case at no cost.♻️ Wider random suffix
- opToken = "auto-" + Long.toString(now(), 16) + "-" + Integer.toString(new Random().nextInt(0xFFFF), 16).padLeft(4, '0') + opToken = "auto-" + Long.toString(now(), 16) + "-" + Long.toString(Math.abs(new Random().nextLong()), 16).padLeft(12, '0')🤖 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 `@hubitat-mcp-server.groovy` around lines 1573 - 1591, Update auto-token generation in the !opTokenActive block to use a wider random suffix than the current 16-bit nextInt(0xFFFF) value, while preserving the existing timestamp prefix, formatting, and token assignment behavior.
2137-2180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the fingerprint is a hash plus length.
_canonicalOpArgs,_findIdenticalRunningOp, and_recentRunningWriteOpscarry no comments, while every neighbouring token helper explains its rationale. The non-obvious part is the fingerprint design: the record storesfpHash+fpLeninstead of the canonical JSON, and duplicate detection therefore accepts a small false-positive risk in exchange for a boundedatomicStaterecord. Add one short comment stating that trade-off and the 10-minute recency window.🤖 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 `@hubitat-mcp-server.groovy` around lines 2137 - 2180, Add a brief comment near the fingerprint handling in _canonicalOpArgs or _findIdenticalRunningOp documenting that fpHash plus fpLen avoids storing canonical JSON, keeps atomicState records bounded, and permits a small false-positive risk; also mention that duplicate detection is limited to the 10-minute recency window used by _findIdenticalRunningOp and _recentRunningWriteOps.
1639-1672: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a retry regression test for
too_many_writes_in_flight.
RelayBudgetSpec.groovyalready covers both refusal statuses and no orphan records. Add a test that re-issues the same client-tokened call after the blocking record expires and confirms execution.🤖 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 `@hubitat-mcp-server.groovy` around lines 1639 - 1672, Add a regression test in RelayBudgetSpec.groovy for too_many_writes_in_flight that first verifies refusal, advances or waits until the blocking in-flight record expires, then re-issues the same client-tokened call and asserts it executes successfully. Reuse the existing setup and no-orphan-record assertions, ensuring the expired blocker no longer prevents retry.src/test/groovy/server/RelayBudgetSpec.groovy (1)
195-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case pinning that the concurrency cap still applies to a client-tokened write.
This spec pins that a client token bypasses the fingerprint duplicate check. The implementation deliberately applies the two gates asymmetrically:
_findIdenticalRunningOpruns only whenopTokenAutois true, while themaxConcurrentWritesgate runs for every write leaf (hubitat-mcp-server.groovyLines 1641 and 1657). Without a test, a later refactor could move the cap inside theopTokenAutobranch and let client-tokened writes escape it unnoticed.💚 Suggested additional case
def "a client-tokened write is still subject to maxConcurrentWrites"() { given: settingsMap.enableWrite = true settingsMap.maxConcurrentWrites = 1 atomicStateMap.opTokens = [ 'auto-room-create': [ state: 'running', tool: 'hub_create_room', startedAt: FIXED_NOW - 1000L ] ] def ran = 0 script.metaClass.toolRenameRoom = { Map args -> ran++; [success: true] } when: def response = mcpDriver.callTool('hub_update_room', [ room: 'Den', newName: 'Study', confirm: true, opToken: 'client-retry-2' ]) then: ran == 0 mcpDriver.parseInner(response).status == 'too_many_writes_in_flight' atomicStateMap.opTokens['client-retry-2'] == null }🤖 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 `@src/test/groovy/server/RelayBudgetSpec.groovy` around lines 195 - 220, Add a test alongside the existing client-tokened write spec that sets maxConcurrentWrites to 1, seeds one running operation, and invokes a different write with a client opToken. Assert the tool does not run, the parsed response reports too_many_writes_in_flight, and no client token record is created, preserving the cap independently of fingerprint duplicate handling.tests/e2e_test.py (2)
3567-3582: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap
hub_call_rulestop/start with the same limiter-recovery pattern as the adjacent writes.The pause (
hub_set_rule_paused) and disabled (hub_set_app_disabled) writes surrounding this block route through_status_write, which bounces the app via the watchdog and retries once on an "excessive hub load" limiter trip. The newhub_call_rule(action=stop)/hub_call_rule(action=start)calls are issued directly and have no such recovery, even though they write to the same rule under the same load conditions this test already documents as limiter-prone.Consider routing these two calls through
_status_write(or an equivalent bounce-and-retry wrapper) so a limiter trip degrades to a retry instead of a hard test failure.🤖 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 `@tests/e2e_test.py` around lines 3567 - 3582, Update the stop/start calls in the rule lifecycle test to use the existing _status_write limiter-recovery pattern, or an equivalent watchdog bounce-and-single-retry wrapper. Preserve the current hub_call_rule actions, success assertions, and status polling while ensuring excessive hub load is retried instead of failing immediately.
4187-4191: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the deployment job record during teardown.
If
job_idexists, cancel the job first when it is unfinished, then calldeployment={"op": "delete", "jobId": job_id}.op="cancel"removes created apps but leaves the job record. Catch cleanup errors so teardown does not mask the test failure or leave duplicatejob_namerecords for recovery lookup.🤖 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 `@tests/e2e_test.py` around lines 4187 - 4191, Update the teardown finally block around _delete_native to also clean up job_id: if present and unfinished, cancel it first, then delete the deployment with deployment={"op": "delete", "jobId": job_id}. Catch cleanup errors so teardown preserves the original test failure, and ensure the job record is removed to avoid duplicate job_name recovery records.
🤖 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 `@hubitat-mcp-server.groovy`:
- Around line 8226-8275: The deployment_jobs guide content exists, but its
section name is missing from the hub_get_tool_guide section enum. Update the
enum in the hub_get_tool_guide schema to include deployment_jobs, matching the
existing section returned by getToolGuideSections() and documented in
TOOL_GUIDE.md.
In `@libraries/mcp-app-cloner-lib.groovy`:
- Line 897: Add the optional client-supplied opToken property to the input
schemas for hub_export_native_app in libraries/mcp-app-cloner-lib.groovy at
lines 897-897, hub_delete_bundle in libraries/mcp-bundles-lib.groovy at lines
448-448, and hub_export_bundle in libraries/mcp-bundles-lib.groovy at lines
475-475, matching the existing operation-token type and description conventions
used by their output schemas.
In `@libraries/mcp-code-management-lib.groovy`:
- Line 2998: Update the hub_delete_item input schema alongside its existing
result opToken field to declare an optional opToken input property, including
the same transport-recovery and token-only replay guidance used by the other
write tools. Keep the existing output schema unchanged.
In `@libraries/mcp-custom-rules-lib.groovy`:
- Line 843: Replace the stray “auto-” wording in the opToken schema descriptions
with “Server-assigned automatic operation token” for hub_create_custom_rule,
hub_update_custom_rule, hub_delete_custom_rule, hub_export_custom_rule,
hub_import_custom_rule, and hub_clone_custom_rule in
libraries/mcp-custom-rules-lib.groovy at lines 843, 877, 900, 947, 987, and
1017; hub_create_dashboard, hub_update_dashboard, hub_delete_dashboard, and
hub_clone_dashboard in libraries/mcp-dashboards-lib.groovy at lines 1081, 1111,
1135, and 1155; and hub_delete_debug_logs and hub_set_log_level in
libraries/mcp-debug-logging-lib.groovy at lines 518 and 538. Preserve the
existing replay guidance.
In `@libraries/mcp-deploy-jobs-lib.groovy`:
- Around line 520-530: In libraries/mcp-deploy-jobs-lib.groovy:520-530, update
_deployOpCancel to apply the same sliceLeaseUntil guard used by _deployOpResume
before deleting targets. In libraries/mcp-deploy-jobs-lib.groovy:661-671, update
_deployOpDelete to apply that guard before removing the record and delete only
the targeted job entry rather than rewriting the entire deployJobs map,
preserving concurrent _deploySaveJob updates for other jobs.
- Line 1: Remove the “(issue `#376`)” suffix from the library description in the
McpDeployJobsLib declaration, preserving the rest of the user-visible
description unchanged.
- Around line 43-49: In the alias validation flow, move the knownAliases
registration in the alias-handling block after _deployCheckAliasRefs validates
op.args. Keep duplicate and operation-type checks before argument validation,
and only append the alias once references have passed so an operation cannot
reference its own alias.
- Around line 455-473: Compute and store commitStarted from job.commitStatus
before the status reset loop in the resume flow. Keep the existing detection of
any non-pending commit entry, then remove the later recomputation so phase
assignment uses the pre-reset value and resumes failed commits in committing
rather than staging.
- Around line 271-301: Update the in-flight handling around
_deployReconcileCreateOp so an unreconciled create operation is treated as
ambiguous rather than re-run. When reconciliation returns null, mark the entry
interrupted/failed, set the job to failed with guidance to verify the hub and
resume only with retryInFlight=true or cancel, clear the slice lease, save the
job, and stop processing; preserve adoption for successfully reconciled creates
and existing addActions behavior.
In `@libraries/mcp-devices-lib.groovy`:
- Line 4251: Remove the extra space in the “auto-token” wording across all six
output schema descriptions, including the entries associated with opToken.
Ensure each exposed tool metadata string uses “auto-token” consistently.
- Line 4464: Update the opToken description in the hub_call_device_replace
schema to clarify that the server-assigned token applies only to untokened write
calls, excluding the read-only list_options=true mode; preserve the existing
replay guidance.
In `@libraries/mcp-diagnostics-lib.groovy`:
- Line 2300: Update every opToken schema description at
libraries/mcp-diagnostics-lib.groovy lines 2300, 2326, 2358, 2393, 2420, 2448,
2482, and 2531; libraries/mcp-files-lib.groovy lines 396 and 422; and
libraries/mcp-item-backups-lib.groovy lines 859, 889, and 1008, replacing
“Server-assigned auto- token” with “Server-assigned token” while preserving the
rest of each description.
In `@libraries/mcp-item-backups-lib.groovy`:
- Line 889: Update the affected input schemas to advertise the server-consumed
opToken field in libraries/mcp-item-backups-lib.groovy:889,
libraries/mcp-diagnostics-lib.groovy:2300, 2326, 2358, 2393, 2420, 2448, 2482,
2531, and libraries/mcp-files-lib.groovy:396, 422. For schemas with required
operation parameters, represent token-only replay as a valid alternate input
shape so schema-driven clients can submit only opToken; preserve the existing
parameterized input shape.
In `@libraries/mcp-native-rules-lib.groovy`:
- Around line 13241-13290: Update the multi-operation guard around editOpGroups
to reject same-family singular and plural combinations: treat triggerOpNames or
actionOpNames containing more than one operation as invalid, even when they are
the only non-empty group. Preserve the documented addTriggers plus addActions
bulk pair, while ensuring addTrigger+addTriggers and addAction+addActions throw
before dispatch.
- Around line 9426-9431: Update the write-gate logic used by both hub_set_rule
and hub_set_native_app so deployment requests with args.deployment.op equal to
status bypass the write requirement, while deployment mutations remain gated.
Preserve _deployRouteFromTool’s mixed-argument validation and add regression
coverage for deployment status when enableWrite is false in both schema-only
helpers.
In `@libraries/mcp-self-admin-lib.groovy`:
- Around line 94-100: Align maxConcurrentWrites validation across
libraries/mcp-self-admin-lib.groovy lines 94-100 and hubitat-mcp-server.groovy
lines 435-437: keep the input range at 0..100 and update the validation upper
bound accordingly, then state both bounds in the IllegalArgumentException
message. Update the range definition in hubitat-mcp-server.groovy lines 435-437
only if choosing a different bound; otherwise keep range: "0..100" as the single
source of truth.
In `@libraries/mcp-system-lib.groovy`:
- Around line 212-231: Update the includeRecentOps handling around _opTokenDedup
and the recentOps construction to prevent unauthorized replay of write
operations. Apply the existing read/write and per-tool permission checks before
replay, or filter write tokens and their buffered results from info.recentOps
when the client lacks access, while preserving visibility of authorized
operations.
In `@src/test/groovy/server/ToolDeploymentJobsSpec.groovy`:
- Around line 5-22: Remove the multi-paragraph class-level docblock above the
deployment tests in ToolDeploymentJobsSpec. Preserve the test code and retain
only comments that document non-obvious setup constraints, such as atomic state
behavior.
In `@tests/BAT-rm-native-crud.md`:
- Around line 231-236: Update the test_prompt stopped-state assertion to state
that eventSubscriptionCount is numeric and equals 0, matching the expected
health-response contract and the explicit verification in the test expectation.
Keep the start-state assertion that eventSubscriptionCount is greater than 0
unchanged.
In `@tests/e2e_test.py`:
- Around line 3571-3576: Update the stopped-status verification around
_rule_status_when to use hub_get_rule_health as the authoritative source for the
rule’s stopped state. Cross-check or fall back to its stopped field when
hub_list_rules does not report stopped, while preserving the existing label/name
decoration assertions.
---
Nitpick comments:
In `@hubitat-mcp-server.groovy`:
- Around line 1573-1591: Update auto-token generation in the !opTokenActive
block to use a wider random suffix than the current 16-bit nextInt(0xFFFF)
value, while preserving the existing timestamp prefix, formatting, and token
assignment behavior.
- Around line 2137-2180: Add a brief comment near the fingerprint handling in
_canonicalOpArgs or _findIdenticalRunningOp documenting that fpHash plus fpLen
avoids storing canonical JSON, keeps atomicState records bounded, and permits a
small false-positive risk; also mention that duplicate detection is limited to
the 10-minute recency window used by _findIdenticalRunningOp and
_recentRunningWriteOps.
- Around line 1639-1672: Add a regression test in RelayBudgetSpec.groovy for
too_many_writes_in_flight that first verifies refusal, advances or waits until
the blocking in-flight record expires, then re-issues the same client-tokened
call and asserts it executes successfully. Reuse the existing setup and
no-orphan-record assertions, ensuring the expired blocker no longer prevents
retry.
In `@libraries/mcp-deploy-jobs-lib.groovy`:
- Line 407: Update the jobId construction in _deploySaveJob to append a short
uniqueness suffix to the current timestamp, using an existing random or counter
utility where available. Preserve the “dj-” prefix and timestamp while ensuring
same-millisecond create calls produce distinct IDs.
- Around line 290-300: Assign the mutated statusList back to the appropriate
job.opStatus or job.commitStatus field before calling _deploySaveJob(job) in the
addActions gated path. Apply the same assignment in the adopt branch, preserving
the staging-based field selection used by the existing status-list logic.
- Around line 356-366: Update the worker exception handling around
_deployRunSlice to track consecutive slice exceptions on the job record,
incrementing the count and retaining the latest error. When the count exceeds a
small bounded retry limit, set the job phase to "failed" with the last error so
the existing stillActive check stops re-arming; reset the consecutive-exception
count after a successful slice.
In `@libraries/mcp-native-rules-lib.groovy`:
- Around line 7757-7758: Replace the manual deduplication loops for
uniqueApplied, including the corresponding logic at the second occurrence, with
List.unique(false) on applied. Preserve insertion order and avoid mutating the
original applied list.
- Around line 152-161: Update _setRuleFlatTool() to document the deployment
capability in its flat-mode description and inputSchema, matching the existing
deployment definition and behavior exposed by the gateway schema. Ensure
flat-mode callers can discover and submit deployment without changing dispatch
or operation handling.
In `@src/test/groovy/server/RelayBudgetSpec.groovy`:
- Around line 195-220: Add a test alongside the existing client-tokened write
spec that sets maxConcurrentWrites to 1, seeds one running operation, and
invokes a different write with a client opToken. Assert the tool does not run,
the parsed response reports too_many_writes_in_flight, and no client token
record is created, preserving the cap independently of fingerprint duplicate
handling.
In `@src/test/groovy/server/ToolManageFilesSpec.groovy`:
- Around line 57-74: Extend the hub_list_files tests around the existing
“hub_list_files filters file names by case-insensitive substring” case: invoke
the tool through mcpDriver.callTool with filter in both gateway modes, add an
HTML-fallback assertion for case-insensitive filtering, and add
cursor-pagination coverage proving the filter is applied before pagination.
In `@tests/BAT-v2.md`:
- Line 4627: Add a separate goal-framed BAT scenario for mixed bare-argument
batches that verifies the call refuses fail-loud without partial application,
then retries the same changes through patches successfully. Remove this
rejection requirement from T661 so it remains focused solely on in_progress
resume behavior.
In `@tests/e2e_test.py`:
- Around line 3567-3582: Update the stop/start calls in the rule lifecycle test
to use the existing _status_write limiter-recovery pattern, or an equivalent
watchdog bounce-and-single-retry wrapper. Preserve the current hub_call_rule
actions, success assertions, and status polling while ensuring excessive hub
load is retried instead of failing immediately.
- Around line 4187-4191: Update the teardown finally block around _delete_native
to also clean up job_id: if present and unfinished, cancel it first, then delete
the deployment with deployment={"op": "delete", "jobId": job_id}. Catch cleanup
errors so teardown preserves the original test failure, and ensure the job
record is removed to avoid duplicate job_name recovery records.
🪄 Autofix
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: 4900d3d3-3eb3-4f98-b62f-729ff4360800
📒 Files selected for processing (32)
.github/scripts/e2e_scope.pyTOOL_GUIDE.mddocs/rm_action_subtype_schemas.mdhubitat-mcp-server.groovylibraries/mcp-app-cloner-lib.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-code-management-lib.groovylibraries/mcp-custom-rules-lib.groovylibraries/mcp-dashboards-lib.groovylibraries/mcp-debug-logging-lib.groovylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-devices-lib.groovylibraries/mcp-diagnostics-lib.groovylibraries/mcp-files-lib.groovylibraries/mcp-item-backups-lib.groovylibraries/mcp-native-rules-lib.groovylibraries/mcp-rooms-lib.groovylibraries/mcp-self-admin-lib.groovylibraries/mcp-system-lib.groovylibraries/mcp-variables-lib.groovylibraries/mcp-virtual-devices-lib.groovylibraries/mcp-visual-rules-lib.groovysrc/test/groovy/server/IncludeResolverSpec.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovytests/BAT-rm-native-crud.mdtests/BAT-v2.mdtests/e2e_test.pytests/sandbox_lint.pytools/build-bundle.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/groovy/server/ToolManageFilesSpec.groovy (1)
57-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd HTML fallback coverage for the new filter.
This test covers only JSON responses. The existing HTML fallback test at Lines 147-163 does not provide
filter, so a regression in filtering HTML-derived filenames could pass. Add an HTML case that usesfilter: 'AlP'and verifies the filtered, sorted names.🤖 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 `@src/test/groovy/server/ToolManageFilesSpec.groovy` around lines 57 - 74, Add HTML fallback coverage alongside the existing HTML fallback test, using filter: 'AlP' when calling toolListFiles. Verify that HTML-derived filenames are filtered case-insensitively and returned in the expected sorted order, matching the JSON test’s alpha.txt and ALPINE.csv results.
🤖 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.
Nitpick comments:
In `@src/test/groovy/server/ToolManageFilesSpec.groovy`:
- Around line 57-74: Add HTML fallback coverage alongside the existing HTML
fallback test, using filter: 'AlP' when calling toolListFiles. Verify that
HTML-derived filenames are filtered case-insensitively and returned in the
expected sorted order, matching the JSON test’s alpha.txt and ALPINE.csv
results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70fd6099-f3b5-4807-8cdf-ce15b12f0e03
📒 Files selected for processing (7)
libraries/mcp-discovery-lib.groovysrc/test/groovy/server/ToolAppDriverCodeSpec.groovysrc/test/groovy/server/ToolLibraryCodeSpec.groovysrc/test/groovy/server/ToolListRmRulesSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolVisualRuleRestoreSpec.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/groovy/server/ToolRmNativeCrudSpec.groovy
…erage, schema consistency Engine: alias self-reference fails at validation; unreconcilable interrupted create ops gate behind retryInFlight instead of re-running (duplicate-app risk); commitStarted read before the resume reset loop; lease guards on cancel/delete; statusList written back on gated paths; worker re-arm bounded by a fail streak; jobId carries a random suffix. Gates: deployment op=status recognized by both schema-only helpers so it stays readable with the Write master off; the token-replay short-circuit and the recentOps journal now enforce the masters. Schema/docs: every write tool that returns an opToken now advertises it as input, unified on the FLAT_TRIM'd wording (flat catalog stays ~116KB); auto-token wording fixed; guard extended to same-family singular+plural; maxConcurrentWrites bounds aligned with the settings page; flat-mode hub_set_rule surfaces deployment; e2e stop/start verifies via hub_get_rule_health (authoritative) with limiter recovery.
…2e write-cap backpressure The e2e hub exposed the contract gap: a schedule-only rule carries NO eventSubscriptions section in a perfectly readable statusJson, so absent-section must read 0, with null reserved for the fetch failing. Stopped rules therefore read 0 too (they ARE at zero live subscriptions). T311 and the lifecycle e2e updated to the corrected contract; the start leg of a time-triggered rule asserts the re-armed schedule, not a subscription it never had. The run also proved the write cap refuses a strictly-serial client: severed responses leave hub-side writes running for minutes, so the runner is 'the third writer'. call_tool now treats too_many_writes_in_flight as the designed backpressure signal -- wait-and-re-issue bounded just past the cap's 10-minute record window. Also tightens the 20 opToken input descriptions to the FLAT_TRIM pattern with the handle lifetime stated per the MCP 2026-07-28 Stateful Tools guidance (flat catalog: 27 B visible per site, was 303).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/groovy/server/ToolRmNativeCrudSpec.groovy (1)
4127-4127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove this redundant comment.
Line 4127 does not explain a non-obvious reason. The test name already identifies the action-family case.
As per coding guidelines, comments should explain only non-obvious reasons.
🤖 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 `@src/test/groovy/server/ToolRmNativeCrudSpec.groovy` at line 4127, Remove the redundant comment immediately preceding the action-family test case; leave the test and surrounding trigger guard unchanged.Source: Coding guidelines
🤖 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 `@libraries/mcp-deploy-jobs-lib.groovy`:
- Around line 437-439: Update the jobId generation in the deployment job
creation flow near _deploySaveJob to use java.util.UUID.randomUUID() before the
first save, replacing the 16-bit Random suffix. Preserve the existing
timestamp-based prefix while ensuring IDs are collision-resistant so
atomicState.deployJobs entries are not overwritten.
In `@libraries/mcp-files-lib.groovy`:
- Line 389: Update the opToken descriptions in both schema definitions at the
visible entries near lines 389 and 416 so “8-128 chars, A-Za-z0-9._-” appears
before [[FLAT_TRIM]], keeping the token format visible in flat mode while
preserving the remaining guidance.
---
Nitpick comments:
In `@src/test/groovy/server/ToolRmNativeCrudSpec.groovy`:
- Line 4127: Remove the redundant comment immediately preceding the
action-family test case; leave the test and surrounding trigger guard unchanged.
🪄 Autofix
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: 2e2f7b40-50ba-4cc1-8970-f5e8101053d3
📒 Files selected for processing (29)
hubitat-mcp-server.groovylibraries/mcp-app-cloner-lib.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-code-management-lib.groovylibraries/mcp-custom-rules-lib.groovylibraries/mcp-dashboards-lib.groovylibraries/mcp-debug-logging-lib.groovylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-devices-lib.groovylibraries/mcp-diagnostics-lib.groovylibraries/mcp-files-lib.groovylibraries/mcp-item-backups-lib.groovylibraries/mcp-native-rules-lib.groovylibraries/mcp-rooms-lib.groovylibraries/mcp-self-admin-lib.groovylibraries/mcp-system-lib.groovylibraries/mcp-variables-lib.groovylibraries/mcp-virtual-devices-lib.groovylibraries/mcp-visual-rules-lib.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovysrc/test/groovy/server/OpTokenReplaySpec.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/SetRuleSelfGatewaySpec.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovytests/BAT-rm-native-crud.mdtests/e2e_test.py
🚧 Files skipped from review as they are similar to previous changes (18)
- libraries/mcp-debug-logging-lib.groovy
- libraries/mcp-virtual-devices-lib.groovy
- libraries/mcp-rooms-lib.groovy
- libraries/mcp-variables-lib.groovy
- libraries/mcp-custom-rules-lib.groovy
- libraries/mcp-self-admin-lib.groovy
- tests/BAT-rm-native-crud.md
- libraries/mcp-code-management-lib.groovy
- src/test/groovy/server/ToolDeploymentJobsSpec.groovy
- libraries/mcp-bundles-lib.groovy
- libraries/mcp-app-cloner-lib.groovy
- src/test/groovy/server/RelayBudgetSpec.groovy
- libraries/mcp-system-lib.groovy
- libraries/mcp-dashboards-lib.groovy
- libraries/mcp-devices-lib.groovy
- libraries/mcp-item-backups-lib.groovy
- hubitat-mcp-server.groovy
- libraries/mcp-visual-rules-lib.groovy
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test (strict, gateway)
- GitHub Check: test (strict, flat)
- GitHub Check: test (normal, gateway)
- GitHub Check: test (normal, flat)
- GitHub Check: groovy2x-spock (allow-failure)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.groovy: Use atomicState for thread-safe persistence, state for UI/counters, and compare device IDs as strings with toString().
Add comments only when explaining non-obvious rationale; avoid multi-paragraph docblocks and references to the current PR, issue, or caller.
Do not ship deprecation aliases when renaming non-conforming MCP tools; rename clients and server references in lockstep.
Use unambiguous parameter names such as device_id and user_id, naming complex parameters after their semantic value.
Merge opposite state mutations into one set__ tool, merge filter/projection variants with optional parameters, and avoid consolidation when error modes, safety gates, or payload shapes differ fundamentally.
Every new MCP tool must explicitly classify readOnlyHint, idempotentHint, and openWorldHint; emit destructiveHint for writes and omit it for reads.
Every leaf tool and gateway must have display metadata with a unique Title-Case title and a one-sentence summary of at most 140 characters ending in a period.
Keep annotation hints advisory; enforce read/write permissions through the universal master gates and destructive operations through confirmation and backup checks.
Tool descriptions must begin with a concise summary, include usage and safety guidance, expose implicit constraints, and keep examples in parameter descriptions or the served guide rather than stuffing them into the body.
Use inputSchema objects, enums for fixed values, omit required when no parameters are mandatory, and declare an outputSchema on every new tool definition with nullable fields modeled accurately.
When output schemas are published, return structuredContent for every successful advertised tool result and emit the wire-form schema with recursive required fields removed.
Validation failures must throw IllegalArgumentException before any side effect; runtime operation failures must return a structured [success:false, error, note] result and use isError:true in the MCP...
Files:
src/test/groovy/server/SetRuleSelfGatewaySpec.groovysrc/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovysrc/test/groovy/server/OpTokenReplaySpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovylibraries/mcp-diagnostics-lib.groovylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-native-rules-lib.groovy
src/test/groovy/**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
Every new MCP tool requires both a direct-call unit test and a dispatch-envelope integration test.
Files:
src/test/groovy/server/SetRuleSelfGatewaySpec.groovysrc/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovysrc/test/groovy/server/OpTokenReplaySpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovy
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Apply the Boy Scout rule for small adjacent improvements within the touched area, but ask the user before incorporating large fixes or changes spanning separate subsystems.
Ask before force-pushing, deleting branches, bypassing hooks or signing, editing other contributors' PR descriptions, or touching Z-Wave/Zigbee radios on a live test hub.
**/*: Follow the Boy Scout rule by fixing small adjacent issues in touched files, but ask the user before including large fixes that broaden the review surface.
Never edit protected version, release-bookkeeping, changelog, or PR-guard-controlled content; ask first before destructive Git operations, editing others' PRs, bypassing hooks, or touching live Z-Wave/Zigbee radios.
Files:
src/test/groovy/server/SetRuleSelfGatewaySpec.groovysrc/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovysrc/test/groovy/server/OpTokenReplaySpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovylibraries/mcp-diagnostics-lib.groovytests/e2e_test.pylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-native-rules-lib.groovy
**/*.{groovy,py}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing.
Files:
src/test/groovy/server/SetRuleSelfGatewaySpec.groovysrc/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovysrc/test/groovy/server/OpTokenReplaySpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovylibraries/mcp-diagnostics-lib.groovytests/e2e_test.pylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-native-rules-lib.groovy
libraries/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
libraries/*.groovy: The library(...) declaration must be the first line; keep comments inside methods, use string-literal subscribe/schedule handlers, and do not place preferences, mappings, or file-scope closures in libraries.
Keep each tool's definitions, implementation, domain helpers, classification metadata, and display metadata in its domain library; do not cross-include libraries.
libraries/*.groovy: Library files must begin with thelibrary(...)declaration, keep comments inside methods, use string-literal subscribe/schedule handlers, and avoid preferences, mappings, or file-scope closures.
Per-tool definitions, implementations, domain helpers, classifications, and display metadata belong in the domain library; gateway membership and dispatch cases remain in the main app; libraries must not cross-include one another.
Files:
libraries/mcp-files-lib.groovylibraries/mcp-diagnostics-lib.groovylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-native-rules-lib.groovy
tests/e2e_test.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/e2e_test.py: Add or update e2e coverage for observable tool, dispatch, gateway, transport/protocol, and bug-fix behavior; keep tool names and scenarios synchronized with the server.
Keep Rule Machine e2e scenarios small and grouped by concern; never soft-skip wire-format assertions on relay 504s.Changes observable to live MCP clients, including tool behavior, dispatch, gateways, transport, and bug fixes, require an updated e2e scenario; keep renamed tools and scripts synchronized.
Files:
tests/e2e_test.py
🔇 Additional comments (14)
src/test/groovy/server/SetRuleSelfGatewaySpec.groovy (1)
40-40: LGTM!src/test/groovy/server/HubInfoFieldContractSpec.groovy (1)
98-134: LGTM!src/test/groovy/server/OpTokenReplaySpec.groovy (1)
153-180: LGTM!tests/e2e_test.py (1)
420-456: LGTM!Also applies to: 3504-3532, 3595-3631, 3644-3672, 3810-3825, 4195-4289, 5911-5942
libraries/mcp-diagnostics-lib.groovy (1)
2295-2302: LGTM!Also applies to: 2322-2329, 2355-2362, 2390-2398, 2418-2426, 2447-2455, 2482-2490, 2533-2540
libraries/mcp-deploy-jobs-lib.groovy (1)
1-9: LGTM!Also applies to: 49-53, 277-313, 377-389, 487-490, 564-569, 705-709, 723-747
libraries/mcp-native-rules-lib.groovy (4)
9270-9274: LGTM!Also applies to: 9309-9312, 9438-9443, 9522-9526, 13253-13304
8-8: LGTM!Also applies to: 24-24, 674-674, 698-698, 818-834, 860-872, 15052-15076
50-57: LGTM!Also applies to: 87-87, 115-115, 139-141, 154-163, 213-213, 285-295, 389-389, 415-417, 466-466, 492-492, 9248-9256
7759-7770: LGTM!Also applies to: 9073-9075
libraries/mcp-files-lib.groovy (1)
6-7: LGTM!Also applies to: 79-82, 111-114, 326-330, 397-397, 424-424
src/test/groovy/server/ToolManageFilesSpec.groovy (1)
57-145: LGTM!Also applies to: 471-471, 692-692, 737-737
src/test/groovy/server/ToolRmNativeCrudSpec.groovy (1)
4096-4126: LGTM!Also applies to: 4128-4152
src/test/groovy/server/UpdateNativeAppSchemaTrimSpec.groovy (1)
170-170: LGTM!
Two deployment creates in the same millisecond could share a jobId and the second would overwrite the first's record. UUID.randomUUID (sandbox allow-listed) removes the case. A flat-mode client could not see the token format (8-128 chars, A-Za-z0-9._-) -- it sat inside FLAT_TRIM against the critical-formats rule. The format now leads the description on all 30 opToken input sites (flat-visible cost: ~28 bytes per site).
…as writes The auto-token gate and the cap's isWriteLeaf classified purely by getReadOnlyToolNames(), so deployment op='status' polls and guide/discover probes minted running write records, counted toward maxConcurrentWrites (a status poll could be refused by the cap it exists to help diagnose), churned the journal, and two concurrent identical polls could hit duplicate_in_flight. Both sites now consult the same schema-only classifiers the Write master uses.
…rage Engine: a create that COMMITTED but reported failure (stageDisabled leg) now records its app so cancel rolls it back and resume gates behind retryInFlight instead of duplicating; reconcile adopts only on a label match; cancel holds the slice lease across its delete loop; the worker skips background:false jobs and initialize() re-arms it after the blanket unschedule(); failed jobs are never cap-evicted (their createdAppIds/ backupKeys are the rollback handles); manifests are bounded (50 ops, 64KB); setDisabled requires an explicit boolean; worker throws are journaled from the first failure; envelopes report success honestly (failed job, failed rollback) plus background/workerScheduled. Guard: replaceActions joins the multi-op guard; the refusal text stops claiming a silent drop for the shared-branch combos and names both tools. Health: stopped reads statusJson's own state.stopped (the field the stop/start toggle already trusts), markup decoration decides when statusJson is unreadable, plain suffixes never false-flag a rule named '(Stopped)'; list path uses the paused remainder-diff; labels keep one encoding; statusJson read failures land in checkErrors. Files: op-result buffers hidden from hub_list_files by default (includeOpResults:true shows them). recentOpsLimit validates strictly. Flat hub_set_rule schema no longer requires operation alongside a self-contained deployment. Docs: draft:true/includeOps documented, backup gate stated, refusal statuses re-homed, T465 op shape fixed, T311 goal-framed. Tests: 20+ new engine specs (worker streak, cap/prune, adoption, lease, confirm gates, honest envelopes), dispatch-envelope tests through mcpDriver.callTool pinning the read-shaped classifier, settingsApplied dedupe pin, cap default pin, stopped-detection matrix, maxConcurrentWrites validation, list_files exclusion.
kingpanther13
added
the
release:minor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libraries/mcp-deploy-jobs-lib.groovy (1)
697-737: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExpose
job.cancelin the status output.
_deployOpCancelreturnscancelandsuccess:falseon its own response only._deployJobStatusnever readsjob.cancel, so a laterdeployment:{op:'status', jobId}on a partially rolled-back job reportsphase:"cancelled"andsuccess:truewith no mention of the apps that could not be deleted. The operator loses the residual-cleanup list as soon as the cancel response is gone.🐛 Proposed fix
if (job.validation != null) out.validation = job.validation + if (job.cancel instanceof Map) { + out.cancel = job.cancel + if (((job.cancel.failures ?: []) as List)) { + out.success = false + out.error = "Rollback incomplete: ${((List) job.cancel.failures).size()} created app(s) could not be deleted." + } + } // jobError stays for compatibility; error is the runtime-error contract's field.🤖 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 `@libraries/mcp-deploy-jobs-lib.groovy` around lines 697 - 737, Update _deployJobStatus to include the persisted job.cancel data in its status map, preserving the cancellation result—including success:false and any residual cleanup details—when later status requests report a cancelled or partially rolled-back job.
🧹 Nitpick comments (1)
src/test/groovy/server/ToolDeploymentJobsSpec.groovy (1)
347-414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
maxOpsPerCallslice bounding.The worker tests cover throwing slices, streak reset, and the
background:falseopt-out. No test drives a job withmaxOpsPerCallset, so the bounded-slice path in_deployRunSlice(line 293) and the resultingworkerScheduled:truecontinuation are unverified. That path is the durability claim of this layer. A create with three ops andmaxOpsPerCall:1should stop after one op, reportphase:"staging"andworkerScheduled == true, then finish across subsequentdeployJobWorker()calls.🤖 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 `@src/test/groovy/server/ToolDeploymentJobsSpec.groovy` around lines 347 - 414, Add a test in the worker slice coverage around deployJobWorker that seeds a job with three operations and maxOpsPerCall set to 1, then verifies the first deployJobWorker call processes only one operation while preserving phase "staging" and setting workerScheduled true. Invoke deployJobWorker for the remaining continuations and assert the job eventually completes, covering the bounded _deployRunSlice path and scheduled continuation behavior.
🤖 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 `@libraries/mcp-files-lib.groovy`:
- Around line 8-10: Update the includeOpResults validation in the
argument-processing flow to distinguish an omitted key from an explicitly
supplied value. Accept only Boolean values, throw IllegalArgumentException for
explicit non-Boolean inputs such as strings, numbers, or null, and perform this
validation before any File Manager calls; preserve false as the default when the
key is omitted.
In `@src/test/groovy/server/ToolRmNativeCrudSpec.groovy`:
- Around line 4154-4178: Update the nonexistent-app preflight test around
toolSetRule to record every uploadHubFile invocation in a list instead of
silently discarding calls. Add an assertion that this upload-call list is empty,
alongside the existing posts.isEmpty() check, so the test verifies no backup
file is written before the 404 steer.
---
Outside diff comments:
In `@libraries/mcp-deploy-jobs-lib.groovy`:
- Around line 697-737: Update _deployJobStatus to include the persisted
job.cancel data in its status map, preserving the cancellation result—including
success:false and any residual cleanup details—when later status requests report
a cancelled or partially rolled-back job.
---
Nitpick comments:
In `@src/test/groovy/server/ToolDeploymentJobsSpec.groovy`:
- Around line 347-414: Add a test in the worker slice coverage around
deployJobWorker that seeds a job with three operations and maxOpsPerCall set to
1, then verifies the first deployJobWorker call processes only one operation
while preserving phase "staging" and setting workerScheduled true. Invoke
deployJobWorker for the remaining continuations and assert the job eventually
completes, covering the bounded _deployRunSlice path and scheduled continuation
behavior.
🪄 Autofix
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: 00a11eec-517b-4d10-b9f3-01aecef04c7c
📒 Files selected for processing (20)
.github/scripts/e2e_scope.pyTOOL_GUIDE.mdhubitat-mcp-server.groovylibraries/mcp-app-cloner-lib.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-code-management-lib.groovylibraries/mcp-deploy-jobs-lib.groovylibraries/mcp-diagnostics-lib.groovylibraries/mcp-files-lib.groovylibraries/mcp-item-backups-lib.groovylibraries/mcp-native-rules-lib.groovylibraries/mcp-self-admin-lib.groovylibraries/mcp-system-lib.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolUpdateMcpSettingsSpec.groovytests/BAT-rm-native-crud.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/scripts/e2e_scope.py
- tests/BAT-rm-native-crud.md
- libraries/mcp-item-backups-lib.groovy
- libraries/mcp-app-cloner-lib.groovy
- libraries/mcp-self-admin-lib.groovy
- libraries/mcp-system-lib.groovy
- libraries/mcp-diagnostics-lib.groovy
- libraries/mcp-native-rules-lib.groovy
- TOOL_GUIDE.md
- hubitat-mcp-server.groovy
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test (normal, gateway)
- GitHub Check: test (strict, gateway)
- GitHub Check: test (strict, flat)
- GitHub Check: groovy2x-spock (allow-failure)
- GitHub Check: test (normal, flat)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.groovy: Use atomicState for thread-safe persistence, state for UI/counters, and compare device IDs as strings with toString().
Add comments only when explaining non-obvious rationale; avoid multi-paragraph docblocks and references to the current PR, issue, or caller.
Do not ship deprecation aliases when renaming non-conforming MCP tools; rename clients and server references in lockstep.
Use unambiguous parameter names such as device_id and user_id, naming complex parameters after their semantic value.
Merge opposite state mutations into one set__ tool, merge filter/projection variants with optional parameters, and avoid consolidation when error modes, safety gates, or payload shapes differ fundamentally.
Every new MCP tool must explicitly classify readOnlyHint, idempotentHint, and openWorldHint; emit destructiveHint for writes and omit it for reads.
Every leaf tool and gateway must have display metadata with a unique Title-Case title and a one-sentence summary of at most 140 characters ending in a period.
Keep annotation hints advisory; enforce read/write permissions through the universal master gates and destructive operations through confirmation and backup checks.
Tool descriptions must begin with a concise summary, include usage and safety guidance, expose implicit constraints, and keep examples in parameter descriptions or the served guide rather than stuffing them into the body.
Use inputSchema objects, enums for fixed values, omit required when no parameters are mandatory, and declare an outputSchema on every new tool definition with nullable fields modeled accurately.
When output schemas are published, return structuredContent for every successful advertised tool result and emit the wire-form schema with recursive required fields removed.
Validation failures must throw IllegalArgumentException before any side effect; runtime operation failures must return a structured [success:false, error, note] result and use isError:true in the MCP...
Files:
src/test/groovy/server/ToolUpdateMcpSettingsSpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovylibraries/mcp-code-management-lib.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-deploy-jobs-lib.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovy
src/test/groovy/**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
Every new MCP tool requires both a direct-call unit test and a dispatch-envelope integration test.
Files:
src/test/groovy/server/ToolUpdateMcpSettingsSpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovy
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Apply the Boy Scout rule for small adjacent improvements within the touched area, but ask the user before incorporating large fixes or changes spanning separate subsystems.
Ask before force-pushing, deleting branches, bypassing hooks or signing, editing other contributors' PR descriptions, or touching Z-Wave/Zigbee radios on a live test hub.
**/*: Follow the Boy Scout rule by fixing small adjacent issues in touched files, but ask the user before including large fixes that broaden the review surface.
Never edit protected version, release-bookkeeping, changelog, or PR-guard-controlled content; ask first before destructive Git operations, editing others' PRs, bypassing hooks, or touching live Z-Wave/Zigbee radios.
Files:
src/test/groovy/server/ToolUpdateMcpSettingsSpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovylibraries/mcp-code-management-lib.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-deploy-jobs-lib.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovy
**/*.{groovy,py}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing.
Files:
src/test/groovy/server/ToolUpdateMcpSettingsSpec.groovysrc/test/groovy/server/HubInfoFieldContractSpec.groovylibraries/mcp-files-lib.groovylibraries/mcp-code-management-lib.groovysrc/test/groovy/server/RelayBudgetSpec.groovysrc/test/groovy/server/ToolManageFilesSpec.groovysrc/test/groovy/server/ToolRmNativeCrudSpec.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-deploy-jobs-lib.groovysrc/test/groovy/server/ToolDeploymentJobsSpec.groovy
libraries/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
libraries/*.groovy: The library(...) declaration must be the first line; keep comments inside methods, use string-literal subscribe/schedule handlers, and do not place preferences, mappings, or file-scope closures in libraries.
Keep each tool's definitions, implementation, domain helpers, classification metadata, and display metadata in its domain library; do not cross-include libraries.
libraries/*.groovy: Library files must begin with thelibrary(...)declaration, keep comments inside methods, use string-literal subscribe/schedule handlers, and avoid preferences, mappings, or file-scope closures.
Per-tool definitions, implementations, domain helpers, classifications, and display metadata belong in the domain library; gateway membership and dispatch cases remain in the main app; libraries must not cross-include one another.
Files:
libraries/mcp-files-lib.groovylibraries/mcp-code-management-lib.groovylibraries/mcp-bundles-lib.groovylibraries/mcp-deploy-jobs-lib.groovy
🔇 Additional comments (35)
src/test/groovy/server/ToolUpdateMcpSettingsSpec.groovy (2)
548-568: LGTM!
570-581: LGTM!src/test/groovy/server/HubInfoFieldContractSpec.groovy (1)
136-158: LGTM!Also applies to: 160-180
src/test/groovy/server/RelayBudgetSpec.groovy (1)
39-44: LGTM!Also applies to: 81-92, 96-117, 119-147, 149-176, 178-206, 208-240, 242-269, 271-297
libraries/mcp-bundles-lib.groovy (1)
392-399: LGTM!Also applies to: 441-449, 470-477
libraries/mcp-code-management-lib.groovy (1)
2764-2771: LGTM!Also applies to: 2822-2829, 2878-2885, 2943-2950, 2991-2999, 3032-3039, 3074-3081
libraries/mcp-files-lib.groovy (1)
82-88: LGTM!Also applies to: 117-123, 335-340, 399-407, 426-434
libraries/mcp-deploy-jobs-lib.groovy (13)
40-58: LGTM!
60-68: LGTM!
99-151: LGTM!
153-221: LGTM!
223-268: LGTM!
270-401: LGTM!
403-452: LGTM!
467-501: LGTM!
502-532: LGTM!
534-623: LGTM!
625-684: LGTM!
756-788: LGTM!
790-835: LGTM!src/test/groovy/server/ToolDeploymentJobsSpec.groovy (5)
29-47: LGTM!
49-179: LGTM!Also applies to: 181-277
279-345: LGTM!Also applies to: 416-490
492-596: LGTM!Also applies to: 598-688
690-750: LGTM!Also applies to: 752-820
src/test/groovy/server/ToolManageFilesSpec.groovy (7)
57-80: LGTM!
82-98: LGTM!Also applies to: 100-124
126-143: LGTM!
145-170: LGTM!
496-496: LGTM!
717-717: LGTM!
762-762: LGTM!src/test/groovy/server/ToolRmNativeCrudSpec.groovy (3)
4181-4227: LGTM!
9422-9514: LGTM!
15116-15180: LGTM!
…ateMapValue The checkpoint save assumed atomicState.updateMapValue exists (firmware 2.3.2+); _opTokenPut already carries the MissingMethodException fallback for exactly this. Also: includeOpResults validates strictly, and the 404 preflight test pins that no backup upload happened either.
…equest Root cause of the e2e regression, measured: _opTokenPrune runs on every tokened call, and past 100 records it batch-evicts ~50 -- deleting each result file with a synchronous HTTP call. Before auto-tokens only client-tokened calls made records, so that batch was rare; now every write makes one, so the eviction fires repeatedly and whichever write triggers it eats ~50 deletions. A sub-second hub_delete_variable took 7.7-10.4s (baseline: zero slow variable ops), the median hub_set_rule edit moved 9.5s -> 10.3s across the ~10s relay ceiling, and calls at the ceiling doubled (33 -> 74). Common-test time went 17.3 -> 37.0 min. The prune now queues the deterministic filenames and schedules opTokenFileSweep, which deletes them 20 per pass outside any request, re-arming while work remains; the queue is capped at 500. Two compounding fixes: the write-cap window drops 600s -> 90s, so a record orphaned by a severed write (its hub-side thread dies before the terminal path, leaving it 'running' forever) can no longer wedge every write for ten minutes -- that wedge cost 82 refusals and ~14 minutes of client waiting in this run. And the per-request gateway/read-only aggregator rebuilds are hoisted from nine calls to two.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/test/groovy/server/OpTokenReplaySpec.groovy`:
- Around line 893-899: Update the test around opTokenFileSweep() to assert the
intermediate batch state immediately after the second sweep: deleted must
contain 40 files and atomicStateMap.opTokenSweepFiles must contain 12 queued
files, before performing any further sweep or final drain assertions.
🪄 Autofix
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: 71840725-6970-442c-86ef-88166b3fe503
📒 Files selected for processing (3)
hubitat-mcp-server.groovysrc/test/groovy/server/OpTokenReplaySpec.groovytests/e2e_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e_test.py
- hubitat-mcp-server.groovy
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: e2e (run)
- GitHub Check: test (strict, gateway)
- GitHub Check: test (normal, flat)
- GitHub Check: test (strict, flat)
- GitHub Check: test (normal, gateway)
- GitHub Check: groovy2x-spock (allow-failure)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.groovy: Use atomicState for thread-safe persistence, state for UI/counters, and compare device IDs as strings with toString().
Add comments only when explaining non-obvious rationale; avoid multi-paragraph docblocks and references to the current PR, issue, or caller.
Do not ship deprecation aliases when renaming non-conforming MCP tools; rename clients and server references in lockstep.
Use unambiguous parameter names such as device_id and user_id, naming complex parameters after their semantic value.
Merge opposite state mutations into one set__ tool, merge filter/projection variants with optional parameters, and avoid consolidation when error modes, safety gates, or payload shapes differ fundamentally.
Every new MCP tool must explicitly classify readOnlyHint, idempotentHint, and openWorldHint; emit destructiveHint for writes and omit it for reads.
Every leaf tool and gateway must have display metadata with a unique Title-Case title and a one-sentence summary of at most 140 characters ending in a period.
Keep annotation hints advisory; enforce read/write permissions through the universal master gates and destructive operations through confirmation and backup checks.
Tool descriptions must begin with a concise summary, include usage and safety guidance, expose implicit constraints, and keep examples in parameter descriptions or the served guide rather than stuffing them into the body.
Use inputSchema objects, enums for fixed values, omit required when no parameters are mandatory, and declare an outputSchema on every new tool definition with nullable fields modeled accurately.
When output schemas are published, return structuredContent for every successful advertised tool result and emit the wire-form schema with recursive required fields removed.
Validation failures must throw IllegalArgumentException before any side effect; runtime operation failures must return a structured [success:false, error, note] result and use isError:true in the MCP...
Files:
src/test/groovy/server/OpTokenReplaySpec.groovy
src/test/groovy/**/*.groovy
📄 CodeRabbit inference engine (CLAUDE.md)
Every new MCP tool requires both a direct-call unit test and a dispatch-envelope integration test.
Files:
src/test/groovy/server/OpTokenReplaySpec.groovy
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Apply the Boy Scout rule for small adjacent improvements within the touched area, but ask the user before incorporating large fixes or changes spanning separate subsystems.
Ask before force-pushing, deleting branches, bypassing hooks or signing, editing other contributors' PR descriptions, or touching Z-Wave/Zigbee radios on a live test hub.
**/*: Follow the Boy Scout rule by fixing small adjacent issues in touched files, but ask the user before including large fixes that broaden the review surface.
Never edit protected version, release-bookkeeping, changelog, or PR-guard-controlled content; ask first before destructive Git operations, editing others' PRs, bypassing hooks, or touching live Z-Wave/Zigbee radios.
Files:
src/test/groovy/server/OpTokenReplaySpec.groovy
**/*.{groovy,py}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing.
Files:
src/test/groovy/server/OpTokenReplaySpec.groovy
🔇 Additional comments (5)
src/test/groovy/server/OpTokenReplaySpec.groovy (5)
153-180: LGTM!
848-862: LGTM!
864-892: LGTM!
902-917: LGTM!
959-961: LGTM!
The sweep queue stores these names in atomicState, and a GString never equals the String it renders as -- List.contains and any other value comparison against a stored name silently answers false (caught by the Groovy 2.5 lane, where the spec's contains() check failed against a list that visibly held the name).
It lowered the cap to 1 BEFORE creating its rule, so its own setup writes then queued behind the background leg -- four BACKPRESSURE waits in the last run -- and a 504 on the finally's cap restore failed the whole test. Create the rule first, lower the cap second, and re-send the cap writes on a dropped response (they are idempotent settings assignments).
Independent analysis (codex) reached the same conclusion and declined to rewrite it: the first write replaces its own `running` marker with `complete` in its handler's finally, and a second request cannot enter the app until that handler returns. By the time the journal is inspected there is no running record, so the refusal cannot fire; the relay expiring the queued request is the 504 the last two runs died on. Seeding a running record, as RelayBudgetSpec does deterministically, has no public API. The contract keeps its unit coverage (RelayBudgetSpec, both directions incl. maxConcurrentWrites=0). The suite-pin comment now records WHY there is no live test instead of pointing at one.
My previous commit claimed the refusal was unreachable live. That was wrong, and the analysis behind it was steered -- I handed the reviewing agent my own theory and invited it to agree. Re-asked without that framing, it produced this, which works for reasons my version missed: - it drains any pre-existing running markers first, so a stale marker from an earlier lost-response write cannot occupy the cap slot or impersonate the overlap (that contention is what produced the BACKPRESSURE waits); - it synchronises on the SERVER's marker rather than a sleep, polling the first token until it reads running, so the second write provably arrives during the overlap; - it asserts the refusal names that first token in inFlight, so the refusal must come from this write and not a leftover record. The refusal itself was never in doubt: it was observed live on a hub, and fired 34 times on the 2026-08-09 lane -- which is why the suite pins the cap to 0.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/e2e_test.py`:
- Around line 6095-6107: Update the post-loop assertion following the marker
synchronization loop to accept either marker.get("status") == "running" or
marker.get("replayed") is True, matching both valid break conditions already
handled in that loop. Preserve the existing diagnostic message and polling
behavior.
🪄 Autofix
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: 7e48ffe0-2621-488d-8b93-65e106145004
📒 Files selected for processing (1)
tests/e2e_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: e2e (run)
- GitHub Check: groovy2x-spock
- GitHub Check: test (strict, gateway)
- GitHub Check: test (normal, flat)
- GitHub Check: test (strict, flat)
- GitHub Check: test (normal, gateway)
🧰 Additional context used
📓 Path-based instructions (3)
tests/e2e_test.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/e2e_test.py: Add or update e2e coverage for observable tool, dispatch, gateway, transport/protocol, and bug-fix behavior; keep tool names and scenarios synchronized with the server.
Keep Rule Machine e2e scenarios small and grouped by concern; never soft-skip wire-format assertions on relay 504s.Changes observable to live MCP clients, including tool behavior, dispatch, gateways, transport, and bug fixes, require an updated e2e scenario; keep renamed tools and scripts synchronized.
Files:
tests/e2e_test.py
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Apply the Boy Scout rule for small adjacent improvements within the touched area, but ask the user before incorporating large fixes or changes spanning separate subsystems.
Ask before force-pushing, deleting branches, bypassing hooks or signing, editing other contributors' PR descriptions, or touching Z-Wave/Zigbee radios on a live test hub.
**/*: Follow the Boy Scout rule by fixing small adjacent issues in touched files, but ask the user before including large fixes that broaden the review surface.
Never edit protected version, release-bookkeeping, changelog, or PR-guard-controlled content; ask first before destructive Git operations, editing others' PRs, bypassing hooks, or touching live Z-Wave/Zigbee radios.
Files:
tests/e2e_test.py
**/*.{groovy,py}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing.
Files:
tests/e2e_test.py
🔇 Additional comments (3)
tests/e2e_test.py (3)
28-28: LGTM!
6038-6144: LGTM! The rest of the newtest_write_cap_refuses_a_concurrent_marker_tracked_writecorrectly drains stale running markers before synchronizing on a fresh one, raises and later restoresmaxConcurrentWrites, asserts the refusal names the in-flight token, resolves the background write to a replayable terminal state, and cleans up the background client session and the native rule fixture infinally.
12708-12710: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_e2e_test_helpers.py (1)
1-8: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExercise the JSON-string gateway argument path.
The test passes
argsas a dictionary. It does not detect regressions in JSON decoding. Pass a JSON string instead.Proposed test adjustment
- assert et._op_key("hub_manage_rule_machine", {"tool": "hub_set_rule", "args": {}}) == "hub_set_rule:create" + assert et._op_key("hub_manage_rule_machine", { + "tool": "hub_set_rule", + "args": json.dumps({}), + }) == "hub_set_rule:create"Also applies to: 271-273
🤖 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 `@tests/test_e2e_test_helpers.py` around lines 1 - 8, Update the affected test cases in tests/test_e2e_test_helpers.py to pass the gateway args as a JSON-encoded string rather than a dictionary, exercising the JSON decoding path while preserving the existing expected behavior and assertions.
🤖 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.
Outside diff comments:
In `@tests/test_e2e_test_helpers.py`:
- Around line 1-8: Update the affected test cases in
tests/test_e2e_test_helpers.py to pass the gateway args as a JSON-encoded string
rather than a dictionary, exercising the JSON decoding path while preserving the
existing expected behavior and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c03dd2d-13d3-4a39-a57e-244f0699fdfa
📒 Files selected for processing (4)
hubitat-mcp-server.groovysrc/test/groovy/server/OpTokenReplaySpec.groovytests/e2e_test.pytests/test_e2e_test_helpers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/groovy/server/OpTokenReplaySpec.groovy
- tests/e2e_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: e2e (run)
- GitHub Check: groovy2x-spock
- GitHub Check: test (normal, gateway)
- GitHub Check: test (strict, flat)
- GitHub Check: test (normal, flat)
- GitHub Check: test (strict, gateway)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Apply the Boy Scout rule for small adjacent improvements within the touched area, but ask the user before incorporating large fixes or changes spanning separate subsystems.
Ask before force-pushing, deleting branches, bypassing hooks or signing, editing other contributors' PR descriptions, or touching Z-Wave/Zigbee radios on a live test hub.
**/*: Follow the Boy Scout rule by fixing small adjacent issues in touched files, but ask the user before including large fixes that broaden the review surface.
Never edit protected version, release-bookkeeping, changelog, or PR-guard-controlled content; ask first before destructive Git operations, editing others' PRs, bypassing hooks, or touching live Z-Wave/Zigbee radios.
Files:
tests/test_e2e_test_helpers.py
**/*.{groovy,py}
📄 CodeRabbit inference engine (AGENTS.md)
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing.
Files:
tests/test_e2e_test_helpers.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: kingpanther13
Repo: kingpanther13/Hubitat-local-MCP-server PR: 378
File: libraries/mcp-deploy-jobs-lib.groovy:555-571
Timestamp: 2026-08-09T12:53:41.770Z
Learning: In `libraries/mcp-deploy-jobs-lib.groovy`, `atomicState.updateMapValue` is used for per-job deployment checkpoints, but its null-value deletion semantics are undocumented. `_deployOpDelete` therefore rebuilds `atomicState.deployJobs` only for terminal deployment jobs, consistent with the existing terminal-job pruning pattern. Lease guards must prevent cancellation or deletion while an active deployment worker holds `sliceLeaseUntil`, because the worker saves a whole-job snapshot.
Learnt from: kingpanther13
Repo: kingpanther13/Hubitat-local-MCP-server PR: 0
File: :0-0
Timestamp: 2026-08-10T17:02:34.806Z
Learning: In `hubitat-mcp-server.groovy`, token-only operation-result recovery is handled by `_isOpTokenPollShape` in `handleToolsCall` before gateway required-parameter enforcement. `requiredParamsByTool()` is derived from each tool definition’s `inputSchema.required`; preserve these required declarations so the catalog accurately advertises required tool arguments and poll-shape detection remains aligned with runtime validation.
📚 Learning: 2026-08-08T03:29:24.112Z
Learnt from: CR
Repo: kingpanther13/Hubitat-local-MCP-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-08-08T03:29:24.112Z
Learning: Applies to tests/e2e_test.py : Add or update e2e coverage for observable tool, dispatch, gateway, transport/protocol, and bug-fix behavior; keep tool names and scenarios synchronized with the server.
Applied to files:
tests/test_e2e_test_helpers.py
📚 Learning: 2026-08-08T03:29:37.786Z
Learnt from: CR
Repo: kingpanther13/Hubitat-local-MCP-server PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-08T03:29:37.786Z
Learning: Applies to tests/e2e_test.py : Changes observable to live MCP clients, including tool behavior, dispatch, gateways, transport, and bug fixes, require an updated e2e scenario; keep renamed tools and scripts synchronized.
Applied to files:
tests/test_e2e_test_helpers.py
📚 Learning: 2026-08-08T03:29:24.112Z
Learnt from: CR
Repo: kingpanther13/Hubitat-local-MCP-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-08-08T03:29:24.112Z
Learning: Applies to tests/e2e_test.py : Keep Rule Machine e2e scenarios small and grouped by concern; never soft-skip wire-format assertions on relay 504s.
Applied to files:
tests/test_e2e_test_helpers.py
🔇 Additional comments (3)
tests/test_e2e_test_helpers.py (3)
23-112: LGTM!Also applies to: 113-130
113-181: 📐 Maintainability & Code QualityRun the required repository checks before push.
Run
./gradlew testandpython tests/sandbox_lint.pybefore pushing this change.As per coding guidelines:
**/*.{groovy,py}requires./gradlew testandpython tests/sandbox_lint.pybefore pushing.Source: Coding guidelines
133-181: 🗄️ Data Integrity & IntegrationLikely an incorrect or invalid review comment.
|
Superseded by #388, which retains the useful fixes from this PR while replacing the custom continuation protocol with standard MCP requestState handling. The branch is intentionally preserved. |
…R from new mcp spec (#388) ## Summary - Replace custom client-supplied operation tokens and deployment-job recovery with MCP 2026-07-28 `requestState` continuations over Streamable HTTP. - Keep long native Rule Machine/app writes below the cloud relay ceiling by handing claimed work to an internal Hubitat worker, while retaining duplicate prevention and the global concurrent-write cap. - Make the global write cap memory-only overload protection: no per-call durable lease ledger (two hub-DB round trips on every write call removed), same refusal contract and TTL anti-wedge aging, background work still counted through its own durable records. - Serve the reservation machinery's hot atomicState keys from a write-through in-JVM snapshot, eliminating per-call and per-continuation-leg DB read chatter. - Make package deployment asynchronous and request-correlated, with race-safe recovery for an abandoned scheduled-worker marker, and never let a worker throw overwrite an already-persisted deploy outcome for the same request. - Give the SDK conformance proof the same platform-limiter recovery the suite uses (one verified watchdog bounce + one retry), and poll the e2e restore marker through the watchdog so the main app's post-restore recompile no longer manufactures relay 504s. - Carry the newest same-rule edit baseline across worker executions (a JVM mirror beside the manifest, plus a retried file probe), so backup reuse never depends on cross-execution atomicState visibility and a transient read cannot unlink a healthy baseline. - Retain the useful fixes and production `outputSchema` behavior from #378; live E2E intentionally does not exercise the abandoned output-schema feature. - Route regular and official-SDK live MCP coverage through the modern protocol only. ## Type of change - [x] `feat` — new feature or capability - [ ] `fix` — bug fix - [ ] `chore` — maintenance, dependency bump, or housekeeping - [ ] `refactor` — code restructure with no behaviour change - [ ] `docs` — documentation only - [ ] `test` — tests only - [ ] `ci` — CI/CD pipeline change ## Changes - Use standard state-only `input_required` responses and terminal `complete` tool results for slow writes. - Bind request state to the exact tool and canonical arguments, prevent duplicate execution, retain terminal replay, and preserve the configured all-write concurrency limit; numeric-string `appId`s now reach the pre-reservation refusal seam the same way numeric ones do. - Detach indivisible native-app/rule wizard work from relay-bound HTTP requests. - Replace the #378 public `opToken`/deployment-job path with an internal package worker and durable request-specific outcome. - Concurrent-write accounting is in-memory (`WRITE_REQUEST_LEASES` static + TTL sweep); the e2e suite runs with the cap off and one dedicated live test exercises the `too_many_writes_in_flight` refusal under cap 1. - `hub_list_files` rejects a non-string `filter` before any hub I/O; File Manager name filtering is locale-independent. - Same-rule edit baselines reuse reliably across the detached workers' executions; the reuse probe retries once before the discard path may permanently unlink a handle. - Pin the official Python SDK proof to MCP 2026-07-28 and require one high-level `Client.call_tool()` to complete a logical write lasting more than 10 seconds across individually bounded HTTP legs, with capacity-recovery (bounce + retry on a fresh fixture) when the run lands on a limiter-exhausted hub. - Preserve #378's production output-schema definitions and publication behavior while removing all output-schema live E2E scenarios. - Addresses #376. Supersedes #378 and #386. Closes #376. ## Release Notes - Long Rule Machine and native-app writes now continue automatically using the current MCP protocol instead of timing out at the cloud relay boundary. - Write tools respond noticeably faster over the cloud relay: per-call bookkeeping no longer performs hub-database round trips on every write. - Package updates now acknowledge quickly, run in the background, reject duplicate deployment attempts, and publish a request-correlated final outcome. - Concurrent writes remain globally capped to protect the hub from clients that issue large batches or parallel tool calls. ## Testing - 331 Python/doctest tests plus the e2e helper suites (115 focused tests) pass locally. - Sandbox lint and its self-test pass; Python compilation, Ruff, shell syntax checks, bundle build, and diff checks pass. - Groovy 2.4 parsing, Groovy 2.5 Spock, and all four normal/strict flat/gateway unit-test matrices pass in GitHub Actions. - Three fully green full-lane live E2E runs on the test hub, including the official-SDK conformance proof (7/7 scenarios) and the new live write-cap refusal test. The latest run recorded zero relay 504s and zero platform-limiter interventions anywhere in the run (from five bounces and recurring 504s per run at the PR's midpoint), with the full suite completing in ~55 minutes (from ~69). ## Checklist - [x] **Unit tests added for any new MCP tools, regressions, or bug fixes** (required — see [docs/testing.md](docs/testing.md) for the harness + recipes) - [x] **e2e tests added for new tools and/or regression tests added for any bug fix** (see `tests/e2e_test.py`) - [x] Sandbox lint passes: `python tests/sandbox_lint.py` - [ ] `./gradlew test` passes locally (or CI confirms) - [x] Live-hub BAT tests updated if tool behaviour changed (see `tests/BAT-v2.md`) - [x] Documentation updated if user-facing behaviour or tool surface changed - [x] New/renamed MCP tools follow `AGENTS.md` Tool Design Rules (naming, annotations, schema) — no tool was added or renamed; changed contracts follow the same rules. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automatic continuation and replay for long-running writes, replacing client-supplied operation tokens. * Added filtered, case-insensitive file listings with pagination support. * Added Rule Machine “stopped” status and expanded health details. * Added configurable concurrent-write limits and asynchronous package deployment tracking. * Added reusable rollback backups with clearer scope and status reporting. * **Bug Fixes** * Improved backup cleanup, validation, error reporting, and transport-failure recovery. * Improved handling of missing visual-rule definitions and unset system status. * **Documentation** * Updated backup, continuation, deployment, and SDK conformance guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
deploymentargument tohub_set_rule/hub_set_native_app(no new tools): durable multi-app migration jobs that checkpoint on-hub after every op and survive disconnects —op: create|resume|commit|cancel|delete|status.auto-…opToken (buffered result,hub_get_info(includeRecentOps=true)journal). Marker-tracked long-running writes also get canonical-argumentduplicate_in_flightrefusal and participate inmaxConcurrentWritesbackpressure (default 2; 0 disables). A dropped response can be recovered from its retained journal record without the client having pre-arranged a token.patches:[…]) instead of silently running only the first op. Part of Feature request: durable, resumable batch deployments with validation and atomic cutover for native automations #376.Type of change
feat— new feature or capabilityChanges
libraries/mcp-deploy-jobs-lib.groovy, new): staged clone/import/edit ops with alias resolution, on-hub worker slices, a health-check validation gate beforeready_for_commit, commit cutover, cancel rollback (deletes only created apps), anddeletefor finished job records. Same engine from both tools; self-contained call;statusis a pure read. Cancel no longer stalls on a deleted app (existence is read directly instead of through a fetch that throws for a missing app, which conflated "gone" with "unreadable"); a slice's lease is released by the same save that publishes its terminal phase; an unusable created id fails its op rather than passing a bad alias downstream.hubitat-mcp-server.groovy): auto-token on every untokened write;opTokendeclared on every tokened write tool's schema; canonical-args fingerprint poweringduplicate_in_flight;too_many_writes_in_flightbackpressure (self-admin settable, 0 disables); token journal onhub_get_info. A dead write's record is identified exactly (app-instance start stamp) rather than waited out, and the running-record window is 180s. A bounded newest-three terminal-result backstop preserves prompt token-only replay if the scheduled whole-map sweep wins the narrow final-write race against a just-completed record.[[FLAT_TRIM]]defers long optional description tails while preserving basic-call constraints in flat mode and full detail in gateway mode/the served guide. The transform recurses through nested schemas and no marker tokens leak into the catalog. Current CI measures 116,852 B in the 122,000 B tripwire configuration and 118,424 B at its widest, below the hub's 124,000 B cap; opToken constraints remain flat-visible.mcp-native-rules-lib.groovy): 2+ operative families in one EDIT call throw before any write (the documentedaddTriggers+addActionsbulk pair stays allowed);settingsApplieddeduped.hub_get_rule_health: authoritativestoppedboolean (config-page label/status, HTML-stripped), per-rulepaused/disabledstate, and liveeventSubscriptionCount/scheduledJobCount. A rule literally named… (Stopped)is no longer false-flagged, and the runtime suffix is stripped from returned labels so name lookup still resolves.hub_list_files:filtersubstring param, op-result buffers hidden by default unlessincludeOpResults=true, and response-too-large guidance that names the filter.recentOpsLimitis validated beforeidentifyHubblinks the LED; engine text pointers updated to the argument surface.deployment_jobssection (served + TOOL_GUIDE.md);slow_opsrewritten auto-record-first.Release Notes
hub_set_rule/hub_set_native_appaccept adeploymentargument that runs clone/edit/cutover jobs on the hub with checkpoints — a dropped connection does not lose completed progressmaxConcurrentWritesadvanced setting, default 2)patcheshub_get_rule_healthreports paused, disabled, and stopped state plus live subscription/schedule counts;hub_list_filessupports name filtering and hides operation-result buffers by defaultTesting
ToolDeploymentJobsSpec(new, 1,435 lines),OpTokenReplaySpec(+513/-49),RelayBudgetSpec(+343),ToolRmNativeCrudSpec(+420/-1),HubInfoFieldContractSpec(+101), plusToolListRmRulesSpec,ToolManageFilesSpec, andToolUpdateMcpSettingsSpec. The Groovy 2.5 lane keeps its own harness copy, so therunInrecorder is carried in both.test_deployment_job_lifecycleandtest_deployment_manifest_validated_up_frontcover the deployment surface. Committing writes are never transport-replayed after a 504/network/decode failure; the runner surfaces a typed lost-response error and targeted helpers recover through exact token-only replay, state verification, or unique-name/job adoption. The deployment lifecycle polls its create/commit token, adopts a uniquely named job if the create result is still unavailable, and reconciles/resumes a stale lost-create lease.tools/listgets a longer read retry budget, and the serial suite pinsmaxConcurrentWrites: 0outside the dedicated live-cap test.27b6ca6e: 219/219 e2e tests passed in 4,096.3s, including deployment recovery through multiple relay 504s, the genuinely overlappingtoo_many_writes_in_flightrefusal, and untokened-write journal/replay coverage. Official MCP Python SDK conformance passed 8/8 scenarios; all fast CI lanes and CodeRabbit are green.E2E_CHAOS_504: a write commits, its response is replaced with the relay-504 failure, the client replays the buffered result from its token, and the rule gains exactly one action — verified through both a leaf call and a hard-coded gateway envelope.Checklist
tests/e2e_test.py)python tests/sandbox_lint.py./gradlew testpasses locally (or CI confirms)tests/BAT-v2.md)AGENTS.mdTool Design Rules (naming, annotations, schema)Summary by CodeRabbit
New Features
Documentation