diff --git a/.gitattributes b/.gitattributes index 824801e82..738b0bf31 100644 --- a/.gitattributes +++ b/.gitattributes @@ -10,3 +10,9 @@ src/main/resources/META-INF/resources/assets/** linguist-generated # Install scripts -- enforce correct line endings per platform *.sh text eol=lf *.ps1 text eol=crlf + +# Git hooks have no extension, so *.sh above does not reach them. They are LF in +# the repo today only by luck: a CRLF hook is not merely untidy, it fails to +# execute on Linux and macOS ("bad interpreter"), which would silently disarm the +# force-push guard for everyone who activated it. +.githooks/** text eol=lf diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index e8102e208..fad5837a8 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -36,10 +36,10 @@ Closes # ## Checklist -- [ ] My code follows the project's [code style](CONTRIBUTING.md#code-style) +- [ ] My code follows the project's [code style](https://github.com/labsai/EDDI/blob/main/CONTRIBUTING.md#code-style) - [ ] I have added tests that prove my fix/feature works - [ ] Existing tests pass locally (`./mvnw clean verify -DskipITs`) - [ ] I have updated documentation if needed -- [ ] My commit messages follow [conventional commits](CONTRIBUTING.md#commit-convention) +- [ ] My commit messages follow [conventional commits](https://github.com/labsai/EDDI/blob/main/CONTRIBUTING.md#commit-convention) - [ ] I have not committed any secrets, API keys, or tokens - [ ] This PR has a clear, focused scope (one concern per PR) diff --git a/.github/workflows/auto-approve-copilot.yml b/.github/workflows/auto-approve-copilot.yml index 264f0c4d3..c945d18b6 100644 --- a/.github/workflows/auto-approve-copilot.yml +++ b/.github/workflows/auto-approve-copilot.yml @@ -208,6 +208,22 @@ jobs: continue; } + // "Every check that ran was green" is not the same claim as "CI + // ran". A merge-conflicted PR never triggers CI/CD at all, so the + // loop above sees only CodeQL et al. and finds nothing failing — + // and the approval body would then assert that all CI checks + // passed on a commit that was never built. Require the gating + // jobs to be PRESENT by name, so absence is a reason to wait + // rather than a silent pass. + const REQUIRED_CHECKS = ['Build & Test', 'Integration Tests']; + const present = new Set(relevant.map(c => c.name)); + const absent = REQUIRED_CHECKS.filter(name => !present.has(name)); + if (absent.length > 0) { + core.info(`PR #${number}: required check(s) never reported: ${absent.join(', ')} ` + + `(merge conflict, or CI did not trigger). Not approving.`); + continue; + } + await github.rest.pulls.createReview({ owner, repo, pull_number: number, event: 'APPROVE', body: 'Auto-approved: GitHub Copilot reviewed with no unresolved comments and all CI checks passed.\n\n' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e97ba4753..05587646e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,7 @@ jobs: runs-on: ubuntu-latest outputs: code: ${{ steps.result.outputs.code }} + scripts: ${{ steps.result.outputs.scripts }} steps: - name: Checkout uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -49,6 +50,15 @@ jobs: - 'helm/**' - 'mvnw*' - '.mvn/**' + # The installers are the README's headline `curl | bash` path and were + # in NO filter at all, so a PR touching only them skipped the whole + # pipeline — and a skipped required check still satisfies branch + # protection. They do not need the Java build, just the shell lint. + scripts: + - 'install.sh' + - 'install.ps1' + - 'scripts/**' + - '.githooks/**' - name: Resolve id: result @@ -61,8 +71,10 @@ jobs: # with failing tests builds nothing. if [[ "$GITHUB_REF" == refs/tags/* ]]; then echo "code=true" >> $GITHUB_OUTPUT + echo "scripts=true" >> $GITHUB_OUTPUT else echo "code=${{ steps.filter.outputs.code }}" >> $GITHUB_OUTPUT + echo "scripts=${{ steps.filter.outputs.scripts }}" >> $GITHUB_OUTPUT fi - name: Log result @@ -76,6 +88,79 @@ jobs: echo "- ⏭️ Skipping build/test/docker (docs/licenses/config-only change)" >> $GITHUB_STEP_SUMMARY fi + # ─── Job 0b: Shell Lint ───────────────────────────────────────── + # install.sh is 55 KB, install.ps1 is 40 KB, and they are what the README tells + # people to pipe into a shell. Until this job existed nothing checked them at + # all — not a linter, not a test, not even a syntax parse. + shell-lint: + name: Shell Lint + runs-on: ubuntu-latest + needs: detect-changes + if: needs.detect-changes.outputs.scripts == 'true' + + steps: + - name: Checkout + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + + - name: Bash syntax check + # Catches the class of error that makes `curl | bash` fail at the worst + # possible moment — on a user's machine, halfway through an install. + run: | + for f in install.sh .githooks/*; do + [ -f "$f" ] || continue + head -n1 "$f" | grep -qE '^#!.*(bash|sh)' || continue + echo "checking $f" + bash -n "$f" + done + + - name: ShellCheck + # Uses the shellcheck preinstalled on ubuntu-latest rather than a + # third-party action, so there is no extra supply-chain edge to pin. + run: | + shellcheck --version + shellcheck --severity=warning --shell=bash install.sh + + - name: PowerShell syntax check + shell: pwsh + run: | + # Parse-only: never dot-source or run the installer in CI. + # Covers scripts/ too — preflight-local.ps1 is a contributor-facing + # script and was as unchecked as the installers were. + $ErrorActionPreference = 'Stop' + $failed = $false + Get-ChildItem -Path ./install.ps1, ./scripts -Filter *.ps1 -Recurse -ErrorAction SilentlyContinue | + ForEach-Object { + $errors = $null + [System.Management.Automation.Language.Parser]::ParseFile($_.FullName, [ref]$null, [ref]$errors) | Out-Null + if ($errors) { + $failed = $true + $name = Resolve-Path -Relative $_.FullName + $errors | ForEach-Object { Write-Host "::error file=$name,line=$($_.Extent.StartLineNumber)::$($_.Message)" } + } else { + Write-Host "$($_.Name) parses cleanly" + } + } + if ($failed) { exit 1 } + + - name: PSScriptAnalyzer + shell: pwsh + run: | + # Preinstalled on the runner; installing over it only emits a + # "currently in use" warning. + if (-not (Get-Module -ListAvailable -Name PSScriptAnalyzer)) { + Install-Module PSScriptAnalyzer -Force -Scope CurrentUser -ErrorAction Stop + } + $targets = @('./install.ps1') + if (Test-Path ./scripts) { $targets += './scripts' } + # -Path takes ONE string, not an array — passing @(...) fails with + # "Cannot convert 'System.Object[]' to the type 'System.String'". + $found = @() + foreach ($t in $targets) { + $found += Invoke-ScriptAnalyzer -Path $t -Recurse -Severity Error,Warning + } + $found | Format-Table -AutoSize | Out-String | Write-Host + if ($found | Where-Object { $_.Severity -eq 'Error' }) { exit 1 } + # ─── Job 1: Build & Test ──────────────────────────────────────── build-and-test: name: Build & Test diff --git a/.gitignore b/.gitignore index 616ceb341..50ae64c0e 100644 --- a/.gitignore +++ b/.gitignore @@ -57,6 +57,11 @@ fabric.properties docker/data/ /tmp/ +# Grafana runtime state. The monitoring stack uses a named Docker volume and +# provisions from docs/monitoring/, so anything written here is local scratch — +# grafana.db (a 1.4 MB SQLite file) used to be committed from the bind-mount era. +/grafana-data/ + # Kubernetes — user-specific values and Helm packaging output helm/eddi/charts/ *.tgz diff --git a/docker-compose.monitoring.yml b/docker-compose.monitoring.yml index d51619320..dda5eb808 100644 --- a/docker-compose.monitoring.yml +++ b/docker-compose.monitoring.yml @@ -40,6 +40,10 @@ services: - "3000:3000" volumes: - ./docs/monitoring/eddi-grafana-dashboard.json:/var/lib/grafana/dashboards/eddi.json:ro + # The provider globs this directory, so a second dashboard needs no + # provisioning change. Was stranded in a top-level grafana-data/ left over + # from the bind-mount era and provisioned by nothing. + - ./docs/monitoring/eddi-operations-dashboard.json:/var/lib/grafana/dashboards/eddi-operations.json:ro - ./docs/monitoring/grafana-provisioning/datasources:/etc/grafana/provisioning/datasources:ro - ./docs/monitoring/grafana-provisioning/dashboards:/etc/grafana/provisioning/dashboards:ro - grafana-data:/var/lib/grafana diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index 023ca9f9c..3837cc5f9 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -65,9 +65,11 @@ - [Audit Ledger](audit-ledger.md) - [GDPR / CCPA Compliance](gdpr-compliance.md) - [HIPAA Compliance](hipaa-compliance.md) +- [Business Associate Agreement (BAA) Template](templates/baa-template.md) - [EU AI Act Compliance](eu-ai-act-compliance.md) - [Compliance Data Flow](compliance-data-flow.md) - [Incident Response Plan](incident-response.md) +- [Security Review](security-review.md) - [Privacy & Data Processing](../PRIVACY.md) ## Advanced Concepts @@ -84,6 +86,7 @@ - [Setting Up EDDI on AWS with MongoDB Atlas](setup-eddi-on-aws-with-mongodb-atlas.md) - [Release & Versioning Strategy](release-versioning.md) - [Release Signing & Verification](release-signing.md) +- [Release Notes — 6.0.2](release-notes-6.0.2.md) - [Metrics & Monitoring](metrics.md) - [Monitoring & Tracing Guide](monitoring/monitoring-guide.md) - [Log Administration](log-administration.md) diff --git a/docs/changelog.md b/docs/changelog.md index 6654862f0..e9c63cef7 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,240 @@ +## 🧪 test: regression cover for every fix in this branch, and a bug the coverage work found (2026-08-12) + +**Repo:** EDDI (`fix/code-review-defects-and-docs`) + +Writing the tests found a defect in the fix they were written for, which is the argument for writing +them. + +**The flush window.** The eviction fix retains any conversation whose chain positions are still +in flight — queued, in the flush batch, or dead-lettered. But `flush()` published its batch to +`inFlightBatch` *after* draining the queue. Between those two statements the entries were in +**neither** collection, so an eviction landing in that window read their conversations as fully +persisted and re-seeded them — reintroducing the exact duplicate the fix exists to prevent, through +a narrower door. The drain and the publish now happen under the same read lock submitters take, so +eviction cannot observe the intermediate state. No I/O is inside the lock. + +**Every fix whose failure mode a test can express is mutation-checked.** Not merely "a test that +passes" — in each case the fix was reverted and the test confirmed to fail, with the message it +would print to whoever broke it: + +| Fix | Test | Reverting the fix produces | +| --- | --- | --- | +| Sequence eviction | `sequenceEvictionKeepsQueuedConversationsUnique` | `expected <[0, 1]> but was <[0, 0]>` — the duplicate itself | +| In-flight batch retention | `sequenceEvictionRetainsTheInFlightBatch` | `expected <1> but was <-1>` (UNSEQUENCED) | +| Eviction still reclaims | `sequenceEvictionReclaimsPersistedConversations` | guards the opposite failure — a "fix" that never evicts | +| Rate-limiter overflow | `idleBucketRefillsRatherThanLatchingShut` | a bucket that denies every call after a long idle | +| `HUMAN_DECIDES` message | `votePhase_humanDecidesIsRejectedPendingResumePath` | fails if the message claims HUMAN members are unavailable | +| MCP class-list drift | `everyToolClassInThePackageIsListed` | names the `Mcp*Tools` class missing from `TOOL_CLASSES` | +| Link rot | `everyRelativeLinkResolves` | names the file and the target that does not resolve | +| ToC drift | `everyDocIsListedInSummary` | names the unreachable page | +| Inline FQNs | `noInlineFullyQualifiedNames` | names file, line and the offending name | + +The rest are covered differently, and it is worth being exact about how rather than letting the +sentence above imply more than it should: + +- **`SafeHttpClient`'s timeout backstop** has five direct unit cases (`SafeHttpClientTimeoutTest`) + covering the bound, the caller's own timeout winning, and the rebuild preserving method, headers + and body. They are not mutation-checked in the same sense — the defect was an *absent* bound, so + reverting it is what the "unbounded request is bounded" case already asserts. +- **`ConversationStepRunner`'s registration move** has no dedicated test on purpose: only an + `Error` can reach the window, since the intervening call swallows `Exception`. A test would have + to inject a `StackOverflowError` to prove a hardening change, which pins the mechanism rather than + the behaviour. +- **`ThreadLocalRandom`** is behaviour-preserving; the existing selection tests cover it. +- **The installer CI and the rescued dashboard** are verified by the pipeline itself — `shell-lint` + runs against `install.sh` and passes, and the compose mount is exercised by the monitoring stack + rather than by a unit test. + +Two of the tests above deserve note as *class-of-bug* guards rather than single-defect regressions. + +`DocumentationLinksTest` walks every markdown file in the repository and resolves every relative +link. Link rot is invisible to every other check in this build — markdown compiles to nothing, so a +wrong path is indistinguishable from a right one until a human clicks it, which is how 38 of them +accumulated. It found one immediately that the initial sweep had missed: the README banner uses a +repository-root-relative `/screenshots/…`, which a naive resolver sends to the filesystem root. The +link was fine; the resolver was wrong, and now handles the leading `/` the way the forge does. +Documentation *of* link syntax (the `` `![alt](uri)` `` rows in the output-format tables) is excluded +by stripping code spans and fences, not by an ignore list that would rot in turn. + +`ImportStyleTest` and the 575-name cleanup it guards ship on a separate branch, so they are +described in that branch's own entry rather than claimed here. Writing it did change one fact +recorded above: the original audit had under-counted, because its pattern required a package +segment after `java.util`, so `java.util.List` never matched. + +**Two seams were widened for testability, both deliberately.** `RateLimitBucket` became +package-private with a `backdateLastRefill` hook, because a ~107-day idle bucket cannot be reached +through the public API and reflecting into a private field pins the field name rather than the +behaviour. `SafeHttpClient.withDefaultTimeout` became package-private so its five cases can run +without an embedded server — the existing `SafeHttpClientTest` binds a loopback socket in +`@BeforeEach` and therefore only runs where those are available. + + + +--- + + + +## 🧹 chore: close the gaps outside the build — installer CI, link rot, dead code (2026-08-12) + +**Repo:** EDDI (`fix/code-review-defects-and-docs`) + +The second half of the repository review. The pattern in it is worth naming: the engineering +*inside* the pipeline is strong, and nearly every problem found sat in something no automated check +covered. + +**`install.sh` and `install.ps1` had no verification of any kind.** 95 KB of shipped script, the +README's headline `curl … | bash` path, and they were in **no** path filter — so a PR touching only +the installer skipped the entire pipeline, and a skipped required check still satisfies branch +protection. There was no shellcheck, no lint, not even a syntax parse anywhere in `.github/`. A new +`shell-lint` job now runs `bash -n`, ShellCheck (`--severity=warning`, using the runner's own +binary rather than adding a third-party action to pin), a PowerShell **parse-only** check, and +PSScriptAnalyzer. A `scripts` path filter drives it, so installer-only PRs get CI instead of a free +pass. Verified rather than assumed: `install.sh` is already clean at warning severity, so the gate +lands green and any regression is the script's own. + +**The auto-approve workflow treated an absent check as a passing one.** It required every check-run +present on the head commit to be green, but never asserted the gating jobs had *run*. A +merge-conflicted PR never triggers CI/CD at all, so the loop saw only CodeQL et al., found nothing +failing, and would have approved with a body asserting "all CI checks passed" on a commit that was +never built. It now requires `Build & Test` and `Integration Tests` to be present by name. Note this +deliberately still auto-approves docs-only PRs: a job skipped by its `if` still reports a check-run, +so absence means *CI did not run*, not *CI had nothing to do*. + +**A 1.4 MB SQLite file, and a dashboard nobody could see.** The top-level `grafana-data/` was left +over from a bind-mount era — the monitoring stack has since moved to a named Docker volume +provisioned from `docs/monitoring/`, so nothing referenced the directory at all. It held +`grafana.db` (runtime state, committed), superseded provisioning copies, and +`eddi-operations.json`: a genuinely different, richer dashboard ("Operations Command Center", 21 +panels) that was being maintained while being provisioned by nothing. Deleting it would have thrown +away the useful part, so it moved to `docs/monitoring/eddi-operations-dashboard.json`, is mounted +alongside the existing dashboard (the provider globs the directory, so no provisioning change), and +is downloaded by both installers. The rest is gone and the path is now git-ignored. + +**Dead code.** `CannotExecuteException` had no reference anywhere in main or test. `ILogoutEndpoint` +was a JAX-RS interface declaring `/user/isAuthenticated` and `/user/securityType` with **no +implementing class** — the only `@Path("/user")` in the codebase, so those endpoints were advertised +to OpenAPI and served by nothing. Both removed. + +**Inline fully-qualified names** were also found in breach of AGENTS.md §4.7, but that cleanup does +not ship here — it is a separate branch and its own changelog entry, so this one does not claim +work it did not carry. + +**Link rot: 38 broken links, now zero.** Every `planning/*.md` file computed `../` and `../../` as +though it lived under `docs/planning/`, but the directory is at the repo root — so `../../AGENTS.md` +pointed outside the repository. That one mistake accounted for 32 of them; two more were genuinely +stale paths (`LifecycleManager` moved to `lifecycle/internal/`, and a `WebScraperToolSsrfTest` that +no longer exists). The `.gitbook/assets/` directory referenced by the tutorials does not exist at +all, which broke the **onboarding** path specifically: the "creating your first agent" pages linked +to a Postman collection, and `conversations.md`/`httpcalls.md` to sample agents. Rather than +re-point at files that are gone, those now tell the reader to import EDDI's own `/openapi` into +Postman — generated from the running build, so it cannot go stale the way a committed collection +did. The first diagram a new user meets was a broken image; it is now a Mermaid diagram of the +actual config-and-pipeline model (Mermaid already renders in `docs/architecture.md`). + +`docs/SUMMARY.md` was missing `security-review.md` and `release-notes-6.0.2.md`; every page under +`docs/` is now in the table of contents. The PR template's two `CONTRIBUTING.md` links resolve +correctly in a rendered PR body but 404 in the file's own blob view — neither relative form is right +in both, so they are absolute now. + +**Smaller items.** `.githooks/**` gained a `text eol=lf` attribute: `*.sh` does not match an +extensionless hook, so the force-push guard was LF-in-repo by luck, and a CRLF hook does not merely +look untidy — it fails to execute on Linux and macOS, silently disarming itself. Three +`new Random()` allocations on request paths in application-scoped beans became +`ThreadLocalRandom.current()`. + +**And the guard that guards the guard.** `McpToolFilterCoverageTest` pins both directions of the MCP +allowlist, but both start from a hand-maintained `TOOL_CLASSES` list — so a brand-new `Mcp*Tools` +class nobody added would have its tools invisible *and* leave every assertion green, which is the +exact failure mode the file exists to prevent, one level up. The file documented this as the one +thing it could not check. It can: the compiled classes are already on disk next to the ones under +test, so counting them needs no indexing dependency. Mutation-checked by dropping `McpDocTools` from +the list — the new test fails, and it names the missing class. + +--- + + + +## 🛡️ fix: the audit ledger could report itself as tampered, plus four smaller defects from a full-repo review (2026-08-12) + +**Repo:** EDDI (`fix/code-review-defects-and-docs`) + +A critical review of the whole repository. Most of what it looked for was not there — no swallowed +exceptions, no non-thread-safe statics, no mutable state in the singleton lifecycle tasks, zero +`@Disabled` tests across 14,301 of them — so the findings are few but one of them matters. + +**The audit ledger manufactured `ChainStatus.BROKEN` under load.** `AuditLedgerService` caps its +sequence table at 50,000 conversations and used to `clear()` the whole thing on overflow, on the +stated reasoning that re-seeding from `countByConversation` was *"correct, only slower"*. It is not. +Entries sit in the in-memory queue for up to a flush interval (longer while a failing store is being +retried), so a conversation with queued entries has consumed chain positions the store cannot see +yet. Re-seeding from the store count therefore **hands the same position out twice** — and the +verifier grades a duplicate exactly like a gap: *"Reporting INTACT here would hand an auditor a +false assurance."* The `undelivered` table exists precisely so the ledger's own back-pressure cannot +read as tampering, but it only exculpates **gaps**; duplicates had no such channel. On a busy +deployment the queue is never empty, so essentially every overflow produced them. + +The fix replaces the wholesale `clear()` with an eviction that only drops counters whose positions +are all accounted for somewhere a re-seed can see them — persisted in the store, or attributed in +`undelivered`. Conversations still represented in the queue, in the in-flight flush batch, or in the +undelivered table are retained. Three supporting changes make that sound rather than merely +plausible: + +- A `ReentrantReadWriteLock` spans "position consumed" → "entry visible in the queue" on the + submit path (read lock — submitters never contend with each other) against eviction (write lock). + Without it, eviction could still read a conversation as idle while a submitter held a number for + it that nothing could see yet. The window was not theoretical: it contains HMAC and Ed25519 + signing. +- `flush()` publishes the batch it has polled but not yet persisted, because between the poll and a + successful append those positions exist in neither the queue nor the store. It is now + `synchronized` too — the scheduled writer and the `@PreDestroy` final flush could otherwise poll + interleaved halves of the queue into two batches. +- When the table is *still* full after eviction (every counter genuinely in flight), new + conversations get `UNSEQUENCED` rather than a re-seeded collision. That degrades the window to + `UNAVAILABLE` — "the chain cannot be established" — which is honest, where a duplicate is an + accusation. + +One residual case is left deliberately: past `MAX_TRACKED_UNDELIVERED` the undelivered table stops +recording, so a dead-lettered position may be reused. That window already reports `BROKEN` by the +documented fail-strict rule, so the verdict is unchanged — only its reason is. + +Two regression tests, both mutation-checked. With the retain set emptied (simulating the old +`clear()`), `sequenceEvictionKeepsQueuedConversationsUnique` fails with `expected <[0, 1]> but was +<[0, 0]>` — the duplicate itself. Its counterpart pins the opposite direction, so the fix cannot +"pass" by simply never evicting and stranding every later conversation on `UNSEQUENCED`. + +**`HUMAN_DECIDES` blamed a feature that ships.** `AgentGroupStore` rejected the tie policy with +*"needs human group members (I6), which are not available yet"* — roughly 150 lines below its own +"I6 save-time matrix for HUMAN members", which accepts them, validates their `displayName` and warns +about HUMAN moderators. Humans as group members shipped in 10c; what is actually missing is the +resume path a paused tie-break would need. The message now says that. The test pinned the word +"I6", so it was rewritten to assert on the offered alternatives and to fail if the message ever +claims HUMAN members are unavailable again. + +**`SafeHttpClient` documented a guarantee it did not provide.** The class claimed an "overall +wall-clock timeout enforced across all hops", but the budget is only checked *between* hops, so a +single hop that accepts the connection and then trickles its body hung indefinitely and the budget +never fired. Redirect hops already had a 15 s fallback; the initial request had whatever the caller +set, or nothing. Both are now bounded by one `DEFAULT_REQUEST_TIMEOUT`, and the Javadoc states what +is actually true — a per-hop response timeout plus a budget checked between hops. Every in-tree +caller already set its own timeout, so this is a backstop for the next one that does not. + +**A rate-limit bucket could lock shut permanently.** `ToolRateLimiter.refill()` computed +`elapsedNanos * limit` in long arithmetic, which overflows after ~107 idle days at the default limit +of 1000; the wrapped negative drives `tokens` below zero and `tryAcquire` refuses every subsequent +call. One cast. + +**Hardening.** `ConversationStepRunner` registered the in-flight conversation one statement above +the `try` whose `finally` unregisters it. Only an `Error` could strand the entry — the intervening +call swallows `Exception` — but a stranded entry keeps a finished turn's memory reachable and makes +a later cancel signal a dead pipeline, so the registration moved inside. + +--- + + + ## 🔓 fix(csp): the Manager's update check was blocked by our own CSP, in every production deployment (2026-08-12) **Repo:** EDDI (`fix/csp-allow-github-release-check`) diff --git a/docs/conversations.md b/docs/conversations.md index dd691cb85..5003f0410 100644 --- a/docs/conversations.md +++ b/docs/conversations.md @@ -641,4 +641,9 @@ Developer tests: ## Sample Agent -Download the [Weather Agent v2 (Postman collection)](.gitbook/assets/weather_bot_v2.zip) to try the full example. +> **Run it yourself.** The GitBook-hosted Postman collections that used to be linked here +> were lost in the migration. You do not need them: every request is shown inline above, +> and Postman can import EDDI's own spec directly — **Import → Link →** +> `/openapi` (`http://localhost:7070/openapi` for a local install). +> It is generated from the running build, so unlike a committed collection it cannot go +> out of date. The same spec is browsable at `/q/swagger-ui`. diff --git a/docs/creating-your-first-agent/README.md b/docs/creating-your-first-agent/README.md index 587d4310f..4dcdc0037 100644 --- a/docs/creating-your-first-agent/README.md +++ b/docs/creating-your-first-agent/README.md @@ -6,7 +6,40 @@ _Prerequisites: Up and Running instance of **EDDI** (see:_ [_Getting started_](. In order to build an Agent with **EDDI**, you will have to create a few configuration files and `POST` them to the corresponding REST APIs. -![](<../.gitbook/assets/eddi-tech-overview-2 (2).jpg>) +An **Agent** points at one or more **Workflows**, and each Workflow lists the +lifecycle steps to run in order. Every step reads its own JSON configuration, so +what the agent *does* lives in configuration rather than in code: + +```mermaid +flowchart TD + subgraph cfg ["What you POST (JSON configuration)"] + direction LR + A["Agent"] --> W["Workflow
which steps, in what order"] + W --> D["Dictionary"] + W --> B["Behavior Rules"] + W --> H["Http Connector"] + W --> O["Output"] + end + + subgraph run ["What happens on every user message"] + direction TB + I["User input"] --> P["Parser
text → expressions"] + P --> R["Behavior Rules
conditions → actions"] + R --> C["Http Connector
call an API"] + R --> G["Output Generation
pick a reply"] + C --> G + G --> Resp["Reply to the user"] + end + + D -.configures.-> P + B -.configures.-> R + H -.configures.-> C + O -.configures.-> G +``` + +The **actions** emitted by Behavior Rules are the whole orchestration mechanism: +steps never call each other directly, they just react to actions. That is why +adding a capability usually means adding a rule and an output, not writing Java. A agent can consists of the following elements: diff --git a/docs/creating-your-first-agent/creating-your-first-agent-1.md b/docs/creating-your-first-agent/creating-your-first-agent-1.md index 792bbcf43..3fc6af68a 100644 --- a/docs/creating-your-first-agent/creating-your-first-agent-1.md +++ b/docs/creating-your-first-agent/creating-your-first-agent-1.md @@ -610,7 +610,12 @@ By the way you can use the attached **postman collection** below to do all of th 7. Create conversation 8. Say Hello to the agent -Download the [Postman collection](../.gitbook/assets/Creating%20and%20chatting%20with%20a%20bot.postman_collection.json) to run through all the steps above. +> **Run it yourself.** The GitBook-hosted Postman collections that used to be linked here +> were lost in the migration. You do not need them: every request is shown inline above, +> and Postman can import EDDI's own spec directly — **Import → Link →** +> `/openapi` (`http://localhost:7070/openapi` for a local install). +> It is generated from the running build, so unlike a committed collection it cannot go +> out of date. The same spec is browsable at `/q/swagger-ui`. ### External Links diff --git a/docs/creating-your-first-agent/creating-your-first-agent.md b/docs/creating-your-first-agent/creating-your-first-agent.md index 3ac3ac5fe..73d1f62df 100644 --- a/docs/creating-your-first-agent/creating-your-first-agent.md +++ b/docs/creating-your-first-agent/creating-your-first-agent.md @@ -163,7 +163,12 @@ By the way you can use the attached **postman collection** below to do all of th 5. Create conversation 6. Say Hello to the agent -Download the [Postman collection](../.gitbook/assets/Creating%20and%20chatting%20with%20a%20bot.postman_collection.json) to run through all the steps above. +> **Run it yourself.** The GitBook-hosted Postman collections that used to be linked here +> were lost in the migration. You do not need them: every request is shown inline above, +> and Postman can import EDDI's own spec directly — **Import → Link →** +> `/openapi` (`http://localhost:7070/openapi` for a local install). +> It is generated from the running build, so unlike a committed collection it cannot go +> out of date. The same spec is browsable at `/q/swagger-ui`. ### External Links diff --git a/docs/httpcalls.md b/docs/httpcalls.md index df6d57f71..513b08b8a 100644 --- a/docs/httpcalls.md +++ b/docs/httpcalls.md @@ -790,4 +790,9 @@ _Response Code_ ## Full Example -Download the [Weather Agent Postman Collection](.gitbook/assets/EDDI%20-%20Weather%20bot.postman_collection.json) to run the full example. +> **Run it yourself.** The GitBook-hosted Postman collections that used to be linked here +> were lost in the migration. You do not need them: every request is shown inline above, +> and Postman can import EDDI's own spec directly — **Import → Link →** +> `/openapi` (`http://localhost:7070/openapi` for a local install). +> It is generated from the running build, so unlike a committed collection it cannot go +> out of date. The same spec is browsable at `/q/swagger-ui`. diff --git a/grafana-data/dashboards/eddi-operations.json b/docs/monitoring/eddi-operations-dashboard.json similarity index 100% rename from grafana-data/dashboards/eddi-operations.json rename to docs/monitoring/eddi-operations-dashboard.json diff --git a/grafana-data/grafana.db b/grafana-data/grafana.db deleted file mode 100644 index f1dd17ac0..000000000 Binary files a/grafana-data/grafana.db and /dev/null differ diff --git a/grafana-data/provisioning/dashboards/dashboard.yml b/grafana-data/provisioning/dashboards/dashboard.yml deleted file mode 100644 index ac331946a..000000000 --- a/grafana-data/provisioning/dashboards/dashboard.yml +++ /dev/null @@ -1,13 +0,0 @@ -apiVersion: 1 - -providers: - - name: 'EDDI' - orgId: 1 - folder: 'EDDI' - type: file - disableDeletion: false - editable: true - allowUiUpdates: true - options: - path: /var/lib/grafana/dashboards - foldersFromFilesStructure: false diff --git a/grafana-data/provisioning/datasources/prometheus.yml b/grafana-data/provisioning/datasources/prometheus.yml deleted file mode 100644 index af7492822..000000000 --- a/grafana-data/provisioning/datasources/prometheus.yml +++ /dev/null @@ -1,10 +0,0 @@ -apiVersion: 1 - -datasources: - - name: Prometheus - type: prometheus - uid: prometheus - access: proxy - url: http://prometheus:9090 - isDefault: true - editable: true diff --git a/install.ps1 b/install.ps1 index 77f588faf..14fb10b37 100644 --- a/install.ps1 +++ b/install.ps1 @@ -535,7 +535,8 @@ function Get-ComposeFiles { "docs/monitoring/prometheus.yml", "docs/monitoring/grafana-provisioning/dashboards/dashboards.yml", "docs/monitoring/grafana-provisioning/datasources/datasources.yml", - "docs/monitoring/eddi-grafana-dashboard.json" + "docs/monitoring/eddi-grafana-dashboard.json", + "docs/monitoring/eddi-operations-dashboard.json" ) foreach ($mf in $monitoringFiles) { $mfTarget = Join-Path -Path $EddiDir -ChildPath $mf @@ -554,7 +555,16 @@ function Get-ComposeFiles { Write-Information -MessageData "✅" } catch { - Write-Warn "Failed to download $mf (monitoring may not work)" + # Bind-mounted as a FILE by docker-compose.monitoring.yml, so a + # missing one is worse than it sounds: Docker creates a + # *directory* at the mount path and Grafana then fails to + # provision, including the dashboards that did download. Same + # reasoning as the Keycloak realm below. + # -Recurse as well as -Force: what is in the way is most + # likely a DIRECTORY left by a previous run's failed mount, + # and -Force alone will not remove one. + if (Test-Path $mfTarget) { Remove-Item -Path $mfTarget -Recurse -Force -ErrorAction SilentlyContinue } + Write-Fail "Failed to download $mf (required for -WithMonitoring).`n URL: $mfUrl" } } } diff --git a/install.sh b/install.sh index cbe563548..4d99a1bce 100755 --- a/install.sh +++ b/install.sh @@ -620,6 +620,7 @@ resolve_compose_files() { "docs/monitoring/grafana-provisioning/dashboards/dashboards.yml" "docs/monitoring/grafana-provisioning/datasources/datasources.yml" "docs/monitoring/eddi-grafana-dashboard.json" + "docs/monitoring/eddi-operations-dashboard.json" ) for mf in "${monitoring_files[@]}"; do local mf_target="$EDDI_DIR/$mf" @@ -635,7 +636,20 @@ resolve_compose_files() { if curl -fsSL "${mf_url}" -o "$mf_target"; then echo -e "${GREEN}✅${RESET}" else - warn "Failed to download ${mf} (monitoring may not work)" + # Every one of these is bind-mounted as a FILE by + # docker-compose.monitoring.yml, so a missing one is worse than it + # sounds: Docker creates a *directory* at the mount path, and Grafana + # then fails to provision — including the dashboards that did + # download. Same reasoning as the Keycloak realm below; a half-built + # monitoring stack is not better than a refusal to build one. + # -rf, not -f: the thing in the way is most likely a DIRECTORY that a + # previous run's failed mount left behind, and `rm -f` cannot remove + # one. Under `set -e` that turns this cleanup into the script's exit + # point, so the user sees "rm: cannot remove ...: Is a directory" + # instead of the message below, and the stale path survives to break + # the next run too. + rm -rf "$mf_target" + fail "Failed to download ${mf} (required for --with-monitoring).\n URL: ${mf_url}" fi fi done diff --git a/planning/agentic-improvements-plan.md b/planning/agentic-improvements-plan.md index 391f271db..6007ae5bd 100644 --- a/planning/agentic-improvements-plan.md +++ b/planning/agentic-improvements-plan.md @@ -2,7 +2,7 @@ > **Scope.** Capability-based A2A routing, cryptographic agent identity, multimodal context attachments, behavioral counterweights, MCP governance, and session safety (snapshots + forking). > -> **Governing Principles.** All work MUST conform to the Nine Pillars in [`docs/project-philosophy.md`](../project-philosophy.md) and the engine rules in [`AGENTS.md`](../../AGENTS.md). Java is the engine; configuration is logic; security is architecture. +> **Governing Principles.** All work MUST conform to the Nine Pillars in [`docs/project-philosophy.md`](../docs/project-philosophy.md) and the engine rules in [`AGENTS.md`](../AGENTS.md). Java is the engine; configuration is logic; security is architecture. > > **Out of scope (tracked elsewhere).** Memory architecture (see [`memory-architecture-plan.md`](memory-architecture-plan.md)), DAG pipeline (Phase 9), HITL framework (Phase 9b), guardrails (`guardrails-architecture.md`), multi-channel adapters (Phase 11b), visual builder (Phase 13), native image (`native-image-migration.md`). @@ -10,7 +10,7 @@ ## 0. How to read this document -This plan is split into six **Waves** (delivery order) that map to six **Improvements** (topical grouping). The two numberings are kept deliberately distinct to avoid collision with the main roadmap's "Phase N" numbering in [`AGENTS.md` §3](../../AGENTS.md). +This plan is split into six **Waves** (delivery order) that map to six **Improvements** (topical grouping). The two numberings are kept deliberately distinct to avoid collision with the main roadmap's "Phase N" numbering in [`AGENTS.md` §3](../AGENTS.md). | Wave | Improvement | Code status (verified 2026-04-17) | Est. effort | | ------ | ----------------------------------------- | --------------------------------- | ----------- | @@ -21,7 +21,7 @@ This plan is split into six **Waves** (delivery order) that map to six **Improve | Wave 5 | Improvement 3 — Multimodal Attachments | Model only, no pipeline/REST | Medium | | Wave 6 | Improvement 2 — Cryptographic Identity | Signing primitive only | High | -**Verification method.** Every "implemented" claim below has been checked against [src/main/java](../../src/main/java) on branch `fix/security-hardening-6.0.2`. Claims are annotated ✅ (present and wired), ⚠️ (present but not wired end-to-end), ❌ (not present). +**Verification method.** Every "implemented" claim below has been checked against [src/main/java](../src/main/java) on branch `fix/security-hardening-6.0.2`. Claims are annotated ✅ (present and wired), ⚠️ (present but not wired end-to-end), ❌ (not present). --- @@ -31,12 +31,12 @@ This plan is split into six **Waves** (delivery order) that map to six **Improve | Component | Location | Status | Notes | | -------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | ------ | ---------------------------------------------------------------------------------------------- | -| [`CapabilityRegistryService`](../../src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java) | `ai.labs.eddi.configs.agents` | ✅ | In-memory skill index. Rebuilds on agent CRUD. | -| [`CapabilityMatchCondition`](../../src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java) | `ai.labs.eddi.modules.rules.impl.conditions` | ✅ | Behavior-rule condition; wired in `RuleDeserialization`. | -| [`ContentTypeMatcher`](../../src/main/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcher.java) | `ai.labs.eddi.modules.rules.impl.conditions` | ✅ | Matches on attachment MIME type. Wired. | -| [`Attachment`](../../src/main/java/ai/labs/eddi/engine/memory/model/Attachment.java) | `ai.labs.eddi.engine.memory.model` | ⚠️ | Metadata record exists. **Not wired to REST input; not fetched by `LlmTask` for forwarding.** | -| [`AgentSigningService`](../../src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java) | `ai.labs.eddi.configs.agents` | ⚠️ | Ed25519 sign/verify with vault-backed key storage. **Primitive only — no call sites wire it.** | -| [`ToolResponseTruncator`](../../src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java) | `ai.labs.eddi.modules.llm.impl` | ✅ | Character-based truncation with size note. | +| [`CapabilityRegistryService`](../src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java) | `ai.labs.eddi.configs.agents` | ✅ | In-memory skill index. Rebuilds on agent CRUD. | +| [`CapabilityMatchCondition`](../src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java) | `ai.labs.eddi.modules.rules.impl.conditions` | ✅ | Behavior-rule condition; wired in `RuleDeserialization`. | +| [`ContentTypeMatcher`](../src/main/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcher.java) | `ai.labs.eddi.modules.rules.impl.conditions` | ✅ | Matches on attachment MIME type. Wired. | +| [`Attachment`](../src/main/java/ai/labs/eddi/engine/memory/model/Attachment.java) | `ai.labs.eddi.engine.memory.model` | ⚠️ | Metadata record exists. **Not wired to REST input; not fetched by `LlmTask` for forwarding.** | +| [`AgentSigningService`](../src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java) | `ai.labs.eddi.configs.agents` | ⚠️ | Ed25519 sign/verify with vault-backed key storage. **Primitive only — no call sites wire it.** | +| [`ToolResponseTruncator`](../src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java) | `ai.labs.eddi.modules.llm.impl` | ✅ | Character-based truncation with size note. | | `AgentConfiguration.identity`, `.security`, `.capabilities` | `configs/agents/model/AgentConfiguration.java` | ⚠️ | POJO fields exist; `security.*` flags have no consumer. | ### 1.2 Not present (❌) @@ -62,7 +62,7 @@ Every task in every Wave MUST satisfy these rules. Reviewers should reject PRs t ### 2.1 Template syntax -EDDI v6 uses **Qute** (see [`AGENTS.md` §5.1](../../AGENTS.md)). In every config example in this plan and in test fixtures: +EDDI v6 uses **Qute** (see [`AGENTS.md` §5.1](../AGENTS.md)). In every config example in this plan and in test fixtures: - Use **single braces**: `{properties.x}`, not `{{properties.x}}`. - `properties.*` returns **raw values**. Never use `.valueString`, `.valueObject`, etc. @@ -79,7 +79,7 @@ All new fields added to `AgentConfiguration`, `LlmConfiguration`, or any other J - Be optional (nullable, default-valued). - Use `@JsonInclude(JsonInclude.Include.NON_NULL)` on getters (or class-level) so old configs round-trip byte-identical. -- Pass an import round-trip test for a pre-v6 ZIP (use one of the fixtures under [`docs/agent-configs`](../agent-configs/)). +- Pass an import round-trip test for a pre-v6 ZIP (use one of the fixtures under [`docs/agent-configs`](../docs/agent-configs/)). - Be absent from the ZIP export if left at default. ### 2.4 Security of new config fields @@ -488,7 +488,7 @@ When building the chat message, iterate `memory.getCurrentStep().getAttachments( ### 7.5 Behavior rule matching -`ContentTypeMatcher` already matches attachments on `currentStep`. Document in [`docs/behavior-rules.md`](../behavior-rules.md) with example routing `image/*` to an external OCR MCP tool. +`ContentTypeMatcher` already matches attachments on `currentStep`. Document in [`docs/behavior-rules.md`](../docs/behavior-rules.md) with example routing `image/*` to an external OCR MCP tool. ### 7.6 Cost accounting @@ -647,12 +647,12 @@ UI tasks live in **EDDI-Manager**, not this repo. Track in `EDDI-Manager/AGENTS. ### 9.2 Documentation updates (per Wave) -- [`docs/architecture.md`](../architecture.md) — new components and flows. -- [`docs/behavior-rules.md`](../behavior-rules.md) — new condition types with examples. -- [`docs/langchain.md`](../langchain.md) — counterweight, tool-loading strategy, response limits. -- [`docs/security.md`](../security.md) — signing model, replay protection, key rotation. -- [`docs/compliance-data-flow.md`](../compliance-data-flow.md) — attachments in flow diagrams. -- [`docs/changelog.md`](../changelog.md) — per-PR entries per [`AGENTS.md` §2](../../AGENTS.md). +- [`docs/architecture.md`](../docs/architecture.md) — new components and flows. +- [`docs/behavior-rules.md`](../docs/behavior-rules.md) — new condition types with examples. +- [`docs/langchain.md`](../docs/langchain.md) — counterweight, tool-loading strategy, response limits. +- [`docs/security.md`](../docs/security.md) — signing model, replay protection, key rotation. +- [`docs/compliance-data-flow.md`](../docs/compliance-data-flow.md) — attachments in flow diagrams. +- [`docs/changelog.md`](../docs/changelog.md) — per-PR entries per [`AGENTS.md` §2](../AGENTS.md). ### 9.3 MCP server surface additions @@ -712,7 +712,7 @@ A Wave is "done" only when: - [ ] Manager UI surface is designed (implementation may lag). - [ ] Documentation updates merged. - [ ] `docs/changelog.md` entry added. -- [ ] At least one end-to-end demo agent config in [`docs/agent-configs/`](../agent-configs/) exercises the feature. +- [ ] At least one end-to-end demo agent config in [`docs/agent-configs/`](../docs/agent-configs/) exercises the feature. - [ ] Metrics visible on `docs/monitoring/eddi-grafana-dashboard.json`. ### 10.3 Rollback strategy @@ -732,14 +732,14 @@ Each Wave is deployable independently. Feature flags: For any agent picking this up mid-stream: -1. Read [`docs/project-philosophy.md`](../project-philosophy.md) first — non-negotiable. -2. Read [`AGENTS.md`](../../AGENTS.md), especially §4 (Java guidelines) and §5 (agent config authoring). +1. Read [`docs/project-philosophy.md`](../docs/project-philosophy.md) first — non-negotiable. +2. Read [`AGENTS.md`](../AGENTS.md), especially §4 (Java guidelines) and §5 (agent config authoring). 3. Run `git status`, `git branch --show-current`, `git log -5 --oneline`. -4. Check [`docs/changelog.md`](../changelog.md) for recent entries on any of the six Waves. +4. Check [`docs/changelog.md`](../docs/changelog.md) for recent entries on any of the six Waves. 5. Do NOT trust historical "implemented" callouts in older plan revisions without re-verifying via `file_search` / `grep_search`. 6. Do NOT commit to `main`. Create a `feature/agentic-` branch. 7. Every commit MUST build (`./mvnw compile`) and new tests MUST pass (`./mvnw test -Dtest=`). -8. When finishing or switching context, append an entry to [`docs/changelog.md`](../changelog.md) describing what shipped, what's in progress, and what's next. +8. When finishing or switching context, append an entry to [`docs/changelog.md`](../docs/changelog.md) describing what shipped, what's in progress, and what's next. --- @@ -749,7 +749,7 @@ _End of plan. This document is the authoritative source for the six Waves. Super > **Scope**: Multi-agent orchestration, A2A evolution, cryptographic agent identity, multimodal context attachments, and behavioral governance. > -> **Governing Principles**: All changes **must** conform to the [Nine Pillars](../project-philosophy.md). Java is the engine, configuration is logic, security is architecture. +> **Governing Principles**: All changes **must** conform to the [Nine Pillars](../docs/project-philosophy.md). Java is the engine, configuration is logic, security is architecture. > [!IMPORTANT] > **Implementation Status (2026-04-07):** diff --git a/planning/documentation-updates-plan.md b/planning/documentation-updates-plan.md index 82ff648d5..b58c77713 100644 --- a/planning/documentation-updates-plan.md +++ b/planning/documentation-updates-plan.md @@ -4,8 +4,8 @@ ## Prerequisite Reading -1. [`docs/changelog.md`](../changelog.md) — Sprint 1 and 2 entries -2. [`AGENTS.md`](../../AGENTS.md) — The full file, especially §3 Roadmap, §4.4 Tool Security, and the Reusable Infrastructure table +1. [`docs/changelog.md`](../docs/changelog.md) — Sprint 1 and 2 entries +2. [`AGENTS.md`](../AGENTS.md) — The full file, especially §3 Roadmap, §4.4 Tool Security, and the Reusable Infrastructure table --- @@ -112,7 +112,7 @@ EDDI enforces security-by-default in production: - **Security headers** (CSP, X-Frame-Options, X-Content-Type-Options) are set on all responses. - **CI scanning** via CodeQL (SAST) and Trivy (vulnerability scan) on every push. -See [`.env.example`](.env.example) for required environment variables. +See [`.env.example`](../.env.example) for required environment variables. ``` --- diff --git a/planning/memory-architecture-plan.md b/planning/memory-architecture-plan.md index 800e64793..d8be5957d 100644 --- a/planning/memory-architecture-plan.md +++ b/planning/memory-architecture-plan.md @@ -4,7 +4,7 @@ > > **Focus**: This document deals **exclusively** with memory management — how EDDI stores, validates, curates, and consolidates conversational and cross-conversation state. > -> **Governing Principles**: All changes **must** conform to the [Nine Pillars](../project-philosophy.md). Memory machinery is Java infrastructure; memory *policy* is JSON configuration. +> **Governing Principles**: All changes **must** conform to the [Nine Pillars](../docs/project-philosophy.md). Memory machinery is Java infrastructure; memory *policy* is JSON configuration. --- diff --git a/planning/multi-agent-ux-improvements.md b/planning/multi-agent-ux-improvements.md index 995fc9382..76ff2f22a 100644 --- a/planning/multi-agent-ux-improvements.md +++ b/planning/multi-agent-ux-improvements.md @@ -249,8 +249,8 @@ enum FeedbackRating { POSITIVE, NEGATIVE, NEUTRAL } ## 4. References - Source research paper (removed during docs cleanup — distilled into this document) -- [architecture.md](../architecture.md) — EDDI architecture overview -- [project-philosophy.md](../project-philosophy.md) — The Nine Pillars -- [GroupConversationService.java](../../src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java) — Phase-based orchestrator -- [AgentGroupConfiguration.java](../../src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java) — Group configuration model -- [A2ATaskHandler.java](../../src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java) — A2A protocol handler +- [architecture.md](../docs/architecture.md) — EDDI architecture overview +- [project-philosophy.md](../docs/project-philosophy.md) — The Nine Pillars +- [GroupConversationService.java](../src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java) — Phase-based orchestrator +- [AgentGroupConfiguration.java](../src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java) — Group configuration model +- [A2ATaskHandler.java](../src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java) — A2A protocol handler diff --git a/planning/observability-and-pipeline-plan.md b/planning/observability-and-pipeline-plan.md index e932e3269..7412d929c 100644 --- a/planning/observability-and-pipeline-plan.md +++ b/planning/observability-and-pipeline-plan.md @@ -4,9 +4,9 @@ ## Prerequisite Reading -1. [`docs/architecture.md`](../architecture.md) — Pipeline lifecycle, task model -2. [`docs/changelog.md`](../changelog.md) — Recent changes for context -3. [`AGENTS.md`](../../AGENTS.md) — §4.2 "Core Architecture" for the lifecycle pipeline model +1. [`docs/architecture.md`](../docs/architecture.md) — Pipeline lifecycle, task model +2. [`docs/changelog.md`](../docs/changelog.md) — Recent changes for context +3. [`AGENTS.md`](../AGENTS.md) — §4.2 "Core Architecture" for the lifecycle pipeline model --- @@ -28,7 +28,7 @@ ### 1b. Instrument the LifecycleManager -The main execution path is in [`LifecycleManager.executeLifecycle()`](../../src/main/java/ai/labs/eddi/engine/lifecycle/LifecycleManager.java). Add span-per-task: +The main execution path is in [`LifecycleManager.executeLifecycle()`](../src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java). Add span-per-task: ```java Span span = tracer.spanBuilder("eddi.task." + task.getId().name()) @@ -67,7 +67,7 @@ quarkus.otel.service.name=eddi ``` **Key files:** -- `src/main/java/ai/labs/eddi/engine/lifecycle/LifecycleManager.java` +- `src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java` - `src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java` - `src/main/resources/application.properties` diff --git a/planning/security-hardening-remaining.md b/planning/security-hardening-remaining.md index c91bd4fdd..62b556583 100644 --- a/planning/security-hardening-remaining.md +++ b/planning/security-hardening-remaining.md @@ -4,13 +4,13 @@ ## Prerequisite Reading -1. [`docs/changelog.md`](../changelog.md) — Sprint 1 and Sprint 2 entries describe what was already done -2. [`AGENTS.md`](../../AGENTS.md) — §4.4 "Tool Security" for URL validation patterns +1. [`docs/changelog.md`](../docs/changelog.md) — Sprint 1 and Sprint 2 entries describe what was already done +2. [`AGENTS.md`](../AGENTS.md) — §4.4 "Tool Security" for URL validation patterns 3. Key files to understand: - - [`SafeHttpClient.java`](../../src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java) — centralized SSRF-safe HTTP - - [`UrlValidationUtils.java`](../../src/main/java/ai/labs/eddi/modules/llm/tools/UrlValidationUtils.java) — SSRF IP validation - - [`VaultSaltManager.java`](../../src/main/java/ai/labs/eddi/secrets/crypto/VaultSaltManager.java) — per-deployment KEK salt - - [`AuthStartupGuard.java`](../../src/main/java/ai/labs/eddi/engine/security/AuthStartupGuard.java) — production auth enforcement + - [`SafeHttpClient.java`](../src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java) — centralized SSRF-safe HTTP + - [`UrlValidationUtils.java`](../src/main/java/ai/labs/eddi/modules/llm/tools/UrlValidationUtils.java) — SSRF IP validation + - [`VaultSaltManager.java`](../src/main/java/ai/labs/eddi/secrets/crypto/VaultSaltManager.java) — per-deployment KEK salt + - [`AuthStartupGuard.java`](../src/main/java/ai/labs/eddi/engine/security/AuthStartupGuard.java) — production auth enforcement --- @@ -36,7 +36,7 @@ SafeHttpClientTest.java (use com.sun.net.httpserver.HttpServer) **Where:** `src/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTest.java` -**Pattern:** See [`WebScraperToolSsrfTest.java`](../../src/test/java/ai/labs/eddi/modules/llm/tools/impl/WebScraperToolSsrfTest.java) for the embedded-server pattern. +**Pattern:** See [`WebScraperToolTest.java`](../src/test/java/ai/labs/eddi/modules/llm/tools/impl/WebScraperToolTest.java) for the embedded-server pattern. **Implementation note:** The tests need 127.0.0.1 for the embedded server, but `validateUrl()` blocks loopback. Use `send()` (not `sendValidated()`) for the embedded server tests and test the redirect target validation separately. diff --git a/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java b/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java index b77087075..c6c0f550d 100644 --- a/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java +++ b/src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java @@ -289,8 +289,11 @@ public static void validateVotePhases(AgentGroupConfiguration groupConfiguration throw new IllegalArgumentException(path + " with optionsSource EXPLICIT needs at least 2 options"); } if (voteConfig.tiePolicy() == AgentGroupConfiguration.TiePolicy.HUMAN_DECIDES) { - throw new IllegalArgumentException(path + ".tiePolicy HUMAN_DECIDES needs human group members (I6), which are " - + "not available yet — use MODERATOR_DECIDES or NO_DECISION"); + // HUMAN members themselves DO ship (see the I6 matrix above, which + // validates them). What is missing is the resume machinery a tie + // break needs: a tally that stops mid-phase has no pause to re-enter. + throw new IllegalArgumentException(path + ".tiePolicy HUMAN_DECIDES is not supported yet — breaking a tie needs a " + + "resume path that a paused VOTE phase does not have. Use MODERATOR_DECIDES or NO_DECISION"); } for (Map.Entry weight : voteConfig.weights().entrySet()) { // isFinite: NaN passes every < comparison and would poison the diff --git a/src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java b/src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java deleted file mode 100644 index 7738166f4..000000000 --- a/src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright EDDI contributors - * SPDX-License-Identifier: Apache-2.0 - */ -package ai.labs.eddi.engine.api; - -import org.eclipse.microprofile.openapi.annotations.Operation; -import org.eclipse.microprofile.openapi.annotations.tags.Tag; - -import jakarta.ws.rs.GET; -import jakarta.ws.rs.POST; -import jakarta.ws.rs.Path; -import jakarta.ws.rs.Produces; -import jakarta.ws.rs.core.MediaType; -import jakarta.ws.rs.core.Response; - -@Path("/user") -@Produces(MediaType.TEXT_PLAIN) -@Tag(name = "Security / Authentication", description = "Authentication status and session management") -public interface ILogoutEndpoint { - @GET - @Path("/isAuthenticated") - @Operation(description = "Check if current user is authenticated.") - Response isUserAuthenticated(); - - @GET - @Path("/securityType") - @Produces(MediaType.TEXT_PLAIN) - @Operation(description = "Read currently enabled security type.") - Response getSecurityType(); - - @POST - @Path("/logout") - @Operation(description = "Logout current authenticated user.") - void logout(); -} diff --git a/src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java b/src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java index ef32a1c84..e1b9daca5 100644 --- a/src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java +++ b/src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java @@ -25,6 +25,7 @@ import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.nio.file.*; import java.time.Instant; import java.nio.charset.StandardCharsets; @@ -58,12 +59,19 @@ public class AuditLedgerService { static final int DEFAULT_MAX_QUEUE_SIZE = 100_000; /** - * Cap on how many conversations are tracked for sequence assignment at once. On - * overflow the whole table is dropped; the next entry for a conversation then - * re-seeds from the store, which costs one count query and keeps the sequence - * gap-free. + * Threshold at which sequence-counter eviction kicks in. On overflow, counters + * whose positions are all accounted for are evicted and re-seeded from the + * store on next use — see {@link #evictSequenceCountersIfFull} for why only + * those are safe to drop. + *

+ * A threshold, not a hard ceiling: the check in {@link #nextSequence} is not + * atomic with the insert that follows it, so concurrent submitters can + * overshoot it slightly. See the comment there for why that is the right trade. + *

+ * Package-visible so the eviction tests can reach the threshold instead of + * hard-coding a copy of it that would drift. */ - private static final int MAX_TRACKED_CONVERSATIONS = 50_000; + static final int MAX_TRACKED_CONVERSATIONS = 50_000; /** * Cap on how many chain positions are remembered as "consumed but never @@ -72,7 +80,7 @@ public class AuditLedgerService { * is hit the remaining drops go unattributed and the verifier falls back to * reporting them as {@code BROKEN} — the conservative verdict. */ - private static final int MAX_TRACKED_UNDELIVERED = 10_000; + static final int MAX_TRACKED_UNDELIVERED = 10_000; private final IAuditStore auditStore; private final boolean enabled; @@ -103,6 +111,36 @@ public class AuditLedgerService { */ private final ConcurrentHashMap> undelivered = new ConcurrentHashMap<>(); private final AtomicInteger undeliveredTracked = new AtomicInteger(0); + /** + * Serialises sequence-table eviction against sequence assignment. Submitters + * take the read lock (shared, so they never contend with one another) for the + * whole span between consuming a chain position and making the entry visible in + * {@link #queue}; {@link #evictSequenceCountersIfFull} takes the write lock. + * Without it, eviction could drop a counter in that window and the next entry + * would re-seed from a store that has not yet seen the outstanding one. + */ + private final ReentrantReadWriteLock sequenceLock = new ReentrantReadWriteLock(); + /** + * The batch {@link #flush()} has polled off the queue but not yet persisted. + * Those entries own chain positions that are in neither the queue nor the + * store, so eviction has to treat their conversations as live. + */ + private volatile List inFlightBatch = List.of(); + /** + * Bumped once per completed {@link #flush()}. The only event that can change + * whether a counter is evictable, so it is what + * {@link #evictSequenceCountersIfFull} uses to decide a rescan is worthwhile. + */ + private final AtomicLong flushGeneration = new AtomicLong(); + /** + * The {@link #flushGeneration} at which an eviction scan last found nothing to + * evict. Equal values mean "already tried, nothing has moved since". + */ + private final AtomicLong evictionBarrier = new AtomicLong(-1L); + /** + * Counts scans that evicted nothing — see {@link #getFutileEvictionScans()}. + */ + private final AtomicLong futileEvictionScans = new AtomicLong(); private ScheduledExecutorService flushExecutor; @Inject @@ -210,7 +248,17 @@ public void submit(AuditEntry entry) { return; } + // Make room in the sequence table BEFORE taking the assignment lock — + // eviction needs the write lock and this thread is about to hold the read + // lock, which a ReentrantReadWriteLock cannot upgrade. + evictSequenceCountersIfFull(entry.conversationId()); + boolean queued = false; + // Read lock (shared — submitters never contend with each other) spans + // "position consumed" → "entry visible in the queue". Eviction takes the + // write lock, so it can never observe a conversation as idle while this + // thread holds a number for it that nothing can see yet. + sequenceLock.readLock().lock(); try { // Scrub secrets from string values in maps AuditEntry scrubbed = scrubSecrets(entry); @@ -237,6 +285,7 @@ public void submit(AuditEntry entry) { queue.offer(signed); queued = true; } finally { + sequenceLock.readLock().unlock(); if (!queued) { // Signing/scrubbing blew up: give the reservation back rather than // leaking capacity that no entry occupies. @@ -290,6 +339,112 @@ private boolean offerBounded(AuditEntry entry) { return true; } + /** + * Drop sequence counters for conversations that can be re-seeded from the store + * without risk, once the table reaches {@link #MAX_TRACKED_CONVERSATIONS}. + *

+ * The table used to be {@code clear()}ed wholesale, on the reasoning that + * "re-seeding is correct, only slower". It is not: the counter is seeded from + * {@code countByConversation}, which sees only what the store already holds. + * Entries sit in {@link #queue} for up to one flush interval — longer while a + * failing store is being retried — so clearing mid-flight re-issued positions + * those entries had already consumed. Duplicates are graded exactly like gaps + * ({@code ChainStatus.BROKEN}), and unlike a gap there is no exculpatory record + * for them: the ledger would report the deployment as tampered because its own + * bookkeeping wrapped around. + *

+ * A counter is safe to drop only when every position it handed out is already + * accounted for somewhere the re-seed can see: persisted in the store, or + * attributed in {@link #undelivered}. So conversations still represented in the + * queue, in the in-flight flush batch, or in the undelivered table are retained + * and everything else goes. Called under no lock and takes the write lock + * itself, so it cannot run while a submitter holds an assigned-but-not- + * yet-queued position. + *

+ * One residual case is deliberate: past {@link #MAX_TRACKED_UNDELIVERED} the + * undelivered table stops recording, so a dead-lettered position may be reused. + * That window already reports {@code BROKEN} by the documented fail-strict + * rule, so the verdict is unchanged — only its reason is. + *

+ * Conversations with dead-lettered positions stay pinned for the process + * lifetime, and that cannot starve the table. Re-seeding them is not merely + * inconvenient, it is unsound: the seed comes from {@code countByConversation}, + * which counts persisted rows, and a dead-lettered gap makes that count smaller + * than the next free position. With sequences 0-9 where 3 and 5 never landed, + * the count is 8 while the next position is 10 — so a re-seed would hand out 8 + * and 9 a second time. Accounting for the highest known undelivered position + * does not rescue it either ({@code max(8, 6)} is still 8); a sound re-seed + * would need a {@code maxSequence(conversationId)} that {@link IAuditStore} + * does not expose. Retention is therefore correct, and it is bounded: + * {@link #undeliveredTracked} counts sequences, so at most + * {@link #MAX_TRACKED_UNDELIVERED} conversations can be pinned (one sequence + * each, the worst case) out of a {@link #MAX_TRACKED_CONVERSATIONS} table — + * leaving 80% of it evictable. {@code undeliveredPinCannotExhaustTheTable} + * exercises that end to end: a dead-lettered conversation stays pinned while + * the persisted ones around it are reclaimed, a later conversation still + * receives a real position rather than {@code UNSEQUENCED}, and the pinned + * chain resumes past its dead-lettered position instead of reusing it. + */ + private void evictSequenceCountersIfFull(String conversationId) { + // Fast path: nothing to do until the table is full, and a conversation + // already tracked does not grow it. + if (conversationSequences.size() < MAX_TRACKED_CONVERSATIONS || conversationSequences.containsKey(conversationId)) { + return; + } + + // A scan that evicted nothing will evict nothing again until a flush has + // moved entries out of the queue. Without this guard, a table full of + // genuinely live conversations makes EVERY submit for an unseen + // conversation take the write lock and traverse the whole queue (bounded + // at maxQueueSize, 100_000 by default) to reach the same conclusion — + // and because submitters hold the read lock, they all queue behind it. + // The barrier turns a per-submit scan into at most one per flush cycle. + if (evictionBarrier.get() == flushGeneration.get()) { + return; + } + + sequenceLock.writeLock().lock(); + try { + if (conversationSequences.size() < MAX_TRACKED_CONVERSATIONS) { + return; + } + long generation = flushGeneration.get(); + + Set retain = new HashSet<>(undelivered.keySet()); + collectConversationIds(queue, retain); + collectConversationIds(inFlightBatch, retain); + + int before = conversationSequences.size(); + conversationSequences.keySet().removeIf(id -> !retain.contains(id)); + int evicted = before - conversationSequences.size(); + + if (evicted > 0) { + LOGGER.infov("Audit sequence table hit {0} conversations — evicted {1} fully-persisted counters; " + + "{2} retained as in flight", MAX_TRACKED_CONVERSATIONS, evicted, conversationSequences.size()); + } else { + // Read AFTER the scan: a flush that completed while we scanned + // must not be recorded as already-accounted-for, or the next + // genuinely useful scan would be skipped. + evictionBarrier.set(generation); + futileEvictionScans.incrementAndGet(); + LOGGER.warnv("Audit sequence table is full ({0}) and every counter is in flight; not rescanning " + + "until the next flush. New conversations are recorded unsequenced meanwhile.", + MAX_TRACKED_CONVERSATIONS); + } + } finally { + sequenceLock.writeLock().unlock(); + } + } + + /** Add every non-null conversation id in {@code entries} to {@code target}. */ + private static void collectConversationIds(Iterable entries, Set target) { + for (AuditEntry entry : entries) { + if (entry != null && entry.conversationId() != null) { + target.add(entry.conversationId()); + } + } + } + /** * Next 0-based position for {@code conversationId}, or * {@link AuditEntry#UNSEQUENCED} when the entry cannot be chained. @@ -303,11 +458,26 @@ private long nextSequence(String conversationId) { return AuditEntry.UNSEQUENCED; } - if (conversationSequences.size() >= MAX_TRACKED_CONVERSATIONS) { - // Bounded by construction: re-seeding is correct, only slower. - LOGGER.warnv("Audit sequence table hit {0} conversations — resetting; sequences will be re-seeded from the store", - MAX_TRACKED_CONVERSATIONS); - conversationSequences.clear(); + // Still full after eviction means every remaining counter belongs to a + // conversation with entries in flight. Re-seeding one of those from the + // store would hand out a position it already used, and a DUPLICATE is + // reported as BROKEN — the ledger accusing the deployment of tampering + // because its own table filled up. An unsequenced entry instead degrades + // the window to UNAVAILABLE ("cannot be established"), which is honest. + // Deliberately NOT atomic with the computeIfAbsent below. Submitters share + // the read lock, so N concurrent unseen conversations can all observe + // "one slot left" and all insert, overshooting by up to the number of + // concurrent callers. That is why MAX_TRACKED_CONVERSATIONS is a + // threshold that triggers eviction rather than a hard ceiling: the + // overshoot is bounded by concurrency, self-corrects at the next + // eviction, and costs one AtomicLong per excess conversation. Making it + // exact would mean serialising the insert path — a lock on every submit + // for a new conversation — to enforce a bound that is a memory heuristic, + // not a correctness property. + if (!conversationSequences.containsKey(conversationId) && conversationSequences.size() >= MAX_TRACKED_CONVERSATIONS) { + LOGGER.warnv("Audit sequence table is at its {0}-conversation threshold with every counter in flight — " + + "new conversations are recorded unsequenced until it drains", MAX_TRACKED_CONVERSATIONS); + return AuditEntry.UNSEQUENCED; } try { @@ -324,15 +494,31 @@ private long nextSequence(String conversationId) { /** * Flush pending entries to the audit store in a batch. */ - void flush() { + // synchronized: the scheduled writer thread and the @PreDestroy final flush + // can otherwise poll interleaved halves of the queue into two batches, and + // `inFlightBatch` below would only describe one of them. + synchronized void flush() { if (queue.isEmpty()) return; List batch = new ArrayList<>(); - AuditEntry entry; - while ((entry = queue.poll()) != null) { - queueSize.decrementAndGet(); - batch.add(entry); + // Draining and publishing must be atomic WITH RESPECT TO EVICTION: an + // entry that has been polled but not yet published is in neither `queue` + // nor `inFlightBatch`, and an eviction landing in that window would read + // its conversation as fully persisted and re-seed it — reintroducing the + // duplicate this whole mechanism exists to prevent. The read lock is the + // same one submitters hold, so this only ever contends with eviction, and + // no I/O happens inside it. + sequenceLock.readLock().lock(); + try { + AuditEntry entry; + while ((entry = queue.poll()) != null) { + queueSize.decrementAndGet(); + batch.add(entry); + } + inFlightBatch = batch; + } finally { + sequenceLock.readLock().unlock(); } if (!batch.isEmpty()) { @@ -365,6 +551,13 @@ void flush() { writeToDeadLetter(batch); consecutiveFailures.set(0); } + } finally { + // Every outcome has put these positions somewhere eviction can see + // again: persisted, re-queued, or recorded in `undelivered`. + inFlightBatch = List.of(); + // Entries have moved, so a previously futile eviction scan may now + // find something. Bump last, once the move is visible. + flushGeneration.incrementAndGet(); } } } @@ -485,6 +678,20 @@ int getQueueSize() { return queueSize.get(); } + /** Conversations currently holding a sequence counter. */ + int getTrackedConversationCount() { + return conversationSequences.size(); + } + + /** + * How many eviction scans found nothing to evict. The barrier exists so this + * stays near-constant under a full table of live conversations rather than + * growing once per submit, which is what the test asserts. + */ + long getFutileEvictionScans() { + return futileEvictionScans.get(); + } + byte[] getHmacKey() { return hmacKey; } diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java b/src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java index 8a5436509..6645e9665 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java @@ -31,7 +31,10 @@ * metadata, etc.) *

  • Maximum redirect count is capped
    • *
  • Connect timeout is enforced per-hop
    • - *
  • Overall wall-clock timeout is enforced across all hops
    • + *
  • Response timeout is enforced per-hop ({@link #DEFAULT_REQUEST_TIMEOUT} + * when the caller set none)
    • + *
  • Overall wall-clock budget is checked between hops, so a redirect chain + * cannot outlive it by more than one hop's timeout
    • *
  • On cross-origin redirects, Authorization/Cookie headers are stripped
    • * * @@ -51,6 +54,16 @@ public class SafeHttpClient { /** Headers managed by HttpClient — must not be copied to redirect requests. */ private static final Set MANAGED_HEADERS = Set.of("host", "content-length", "connection"); + /** + * Per-hop response timeout applied when the caller set none. The wall-clock + * budget below is only consulted BETWEEN hops, so without a per-request bound a + * single hop that accepts the connection and then trickles (or never completes) + * the response body hangs forever and the budget never gets a chance to fire. + * Every in-tree caller sets its own timeout; this is the backstop for the ones + * that do not. + */ + private static final Duration DEFAULT_REQUEST_TIMEOUT = Duration.ofSeconds(15); + /** Security-sensitive headers stripped on cross-origin redirects. */ private static final Set SENSITIVE_HEADERS = Set.of("authorization", "cookie", "proxy-authorization"); @@ -87,7 +100,7 @@ public SafeHttpClient( */ public HttpResponse send(HttpRequest request, HttpResponse.BodyHandler bodyHandler) throws IOException, InterruptedException { - return sendWithRedirects(request, bodyHandler, 0, Instant.now()); + return sendWithRedirects(withDefaultTimeout(request), bodyHandler, 0, Instant.now()); } /** @@ -107,7 +120,39 @@ public HttpResponse send(HttpRequest request, HttpResponse.BodyHandler public HttpResponse sendValidated(HttpRequest request, HttpResponse.BodyHandler bodyHandler) throws IOException, InterruptedException { UrlValidationUtils.validateUrl(request.uri().toString()); - return sendWithRedirects(request, bodyHandler, 0, Instant.now()); + return sendWithRedirects(withDefaultTimeout(request), bodyHandler, 0, Instant.now()); + } + + /** + * Returns {@code request} unchanged when it already carries a timeout, else a + * copy bounded by {@link #DEFAULT_REQUEST_TIMEOUT}. {@link HttpRequest} is + * immutable, so the bound can only be applied by rebuilding. + *

    + * Package-private so {@code SafeHttpClientTimeoutTest} can pin the rebuild + * without an embedded server — the server-backed cases live in + * {@code SafeHttpClientTest} and only run where loopback sockets are available. + */ + static HttpRequest withDefaultTimeout(HttpRequest request) { + if (request.timeout().isPresent()) { + return request; + } + HttpRequest.Builder builder = HttpRequest.newBuilder(request.uri()) + .timeout(DEFAULT_REQUEST_TIMEOUT) + .method(request.method(), request.bodyPublisher().orElse(HttpRequest.BodyPublishers.noBody())); + request.headers().map().forEach((name, values) -> { + // Same exclusion as copyHeaders: HttpClient owns these and rejects + // an attempt to set them explicitly. + if (!MANAGED_HEADERS.contains(name.toLowerCase())) { + for (String value : values) { + builder.header(name, value); + } + } + }); + request.version().ifPresent(builder::version); + if (request.expectContinue()) { + builder.expectContinue(true); + } + return builder.build(); } private HttpResponse sendWithRedirects(HttpRequest request, HttpResponse.BodyHandler bodyHandler, @@ -153,7 +198,7 @@ private HttpResponse sendWithRedirects(HttpRequest request, HttpResponse. boolean methodPreserved = (statusCode == 307 || statusCode == 308) && !"GET".equals(request.method()); HttpRequest.Builder builder = HttpRequest.newBuilder() .uri(resolvedUri) - .timeout(request.timeout().orElse(Duration.ofSeconds(15))); + .timeout(request.timeout().orElse(DEFAULT_REQUEST_TIMEOUT)); // Copy headers from original request, with security-aware filtering boolean sameOrigin = isSameOrigin(request.uri(), resolvedUri); diff --git a/src/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.java b/src/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.java index 98fc6be36..0ca449506 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.java +++ b/src/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.java @@ -207,11 +207,14 @@ Void runConversationStep(Environment environment, IConversationMemory conversati // #2: register the live memory so cancelConversation can signal the // running pipeline via setCancelled (checked at task boundaries). inFlightConversations.put(conversationId, conversationMemory); - // Carry the agent-level tool-approval config onto memory BEFORE the - // pipeline (LlmTask) runs, so the tool-approval gate can resolve its - // effective config. Transient — never persisted; re-resolved each turn. - conversationService.conversationHitlService.populateToolApprovalsConfig(conversationMemory); try { + // Carry the agent-level tool-approval config onto memory BEFORE the + // pipeline (LlmTask) runs, so the tool-approval gate can resolve its + // effective config. Transient — never persisted; re-resolved each turn. + // Inside the try so an Error here cannot strand the registration above: + // a stranded entry would keep a finished turn's memory reachable AND + // make a later cancel signal the wrong (dead) pipeline. + conversationService.conversationHitlService.populateToolApprovalsConfig(conversationMemory); runGuardedConversationStep(loggingContext, conversationId, environment, conversationMemory, executeConversation, memoryStateAtSubmit, persistedState); } finally { diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java index 9b7354176..b172a265f 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java @@ -33,7 +33,7 @@ import java.net.URI; import java.util.List; import java.util.Map; -import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; import java.util.UUID; import static ai.labs.eddi.engine.model.Deployment.Environment.production; @@ -283,7 +283,10 @@ private UserConversation createUserConversation(String intent, String userId, Ag } private AgentDeployment getRandom(List agentDeployments) { - return agentDeployments.get(new Random().nextInt(agentDeployments.size())); + // ThreadLocalRandom: this picks a deployment per incoming request on an + // application-scoped bean, so a per-call Random both allocates and shares + // its seed lock across concurrent callers. + return agentDeployments.get(ThreadLocalRandom.current().nextInt(agentDeployments.size())); } private AgentTriggerConfiguration getAgentTrigger(String intent) { diff --git a/src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java b/src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java deleted file mode 100644 index e6b07c3cb..000000000 --- a/src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java +++ /dev/null @@ -1,14 +0,0 @@ -/* - * Copyright EDDI contributors - * SPDX-License-Identifier: Apache-2.0 - */ -package ai.labs.eddi.engine.lifecycle.exceptions; - -/** - * @author ginccc - */ -public class CannotExecuteException extends LifecycleException { - public CannotExecuteException(String message) { - super(message); - } -} diff --git a/src/main/java/ai/labs/eddi/engine/memory/model/Data.java b/src/main/java/ai/labs/eddi/engine/memory/model/Data.java index 680245d11..bc6852139 100644 --- a/src/main/java/ai/labs/eddi/engine/memory/model/Data.java +++ b/src/main/java/ai/labs/eddi/engine/memory/model/Data.java @@ -9,7 +9,7 @@ import java.util.Collections; import java.util.Date; import java.util.List; -import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; /** * @author ginccc @@ -45,9 +45,9 @@ public Data(String key, T result, List possibleResults, Date timestamp, boole private T chooseRandomResult(List results) { if (!results.isEmpty()) { - Random random = new Random(); - int randNumber = random.nextInt(results.size()); - return results.get(randNumber); + // ThreadLocalRandom avoids allocating (and seeding) a Random for every + // Data instance constructed during a turn. + return results.get(ThreadLocalRandom.current().nextInt(results.size())); } return null; diff --git a/src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java b/src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java index 8d220664a..7901f3347 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java +++ b/src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java @@ -153,7 +153,11 @@ public void init() { * be called while holding it. *

    */ - private static class RateLimitBucket { + // Package-private (not private) so the overflow regression test can drive it + // directly: reproducing a ~107-day idle bucket through the public API is not + // possible, and the alternative — reflecting into a private field — pins the + // field name rather than the behaviour. + static class RateLimitBucket { private int limit; private double tokens; private long lastRefillNanos; @@ -164,6 +168,21 @@ private static class RateLimitBucket { this.lastRefillNanos = System.nanoTime(); } + /** + * Backdate this bucket's last refill, so idle-window behaviour is provable + * without waiting for the window. Same test seam as + * {@code WorkflowTraversal.discoverConfigs(..., nowMillis)}: an explicit clock + * reading rather than a sleep. + */ + synchronized void backdateLastRefill(long nanosAgo) { + lastRefillNanos -= nanosAgo; + } + + /** Raw token count, for assertions that need sub-integer precision. */ + synchronized double tokenCount() { + return tokens; + } + /** * Re-point this bucket at a new configured limit, preserving how much of the * old allowance had been consumed. Resetting {@code tokens} to the new limit @@ -188,7 +207,12 @@ private void refill() { return; } lastRefillNanos = now; - tokens = Math.min(limit, tokens + elapsedNanos * limit / WINDOW_NANOS); + // (double) on elapsedNanos: `elapsedNanos * limit` is long arithmetic and + // overflows once a bucket has been idle for ~107 days at the default + // limit of 1000. The wrapped negative would drive `tokens` below zero and + // tryAcquire would refuse every call from then on — a bucket that silently + // locks shut rather than refilling. + tokens = Math.min(limit, tokens + (double) elapsedNanos * limit / WINDOW_NANOS); } synchronized boolean tryAcquire() { diff --git a/src/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.java b/src/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.java index ea6a6c5bd..2304fd759 100644 --- a/src/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.java +++ b/src/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.java @@ -36,6 +36,7 @@ import java.net.URI; import java.util.*; +import java.util.concurrent.ThreadLocalRandom; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -250,7 +251,10 @@ private String createOutputKey(String action, List outputValues, St } private OutputItem chooseRandomly(List possibleValues) { - return possibleValues.get(new Random().nextInt(possibleValues.size())); + // ThreadLocalRandom, not `new Random()`: this task is an application-scoped + // singleton on the request path, so a fresh Random per call both allocates + // and contends on its seed across concurrent conversations. + return possibleValues.get(ThreadLocalRandom.current().nextInt(possibleValues.size())); } private int countActionOccurrences(IConversationStepStack conversationStepStack, String action) { diff --git a/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java b/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java index e11d46486..951e0b7fe 100644 --- a/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java +++ b/src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java @@ -142,13 +142,24 @@ void votePhase_explicitWithTooFewOptions_isRejected() { assertTrue(ex.getMessage().contains("2 options"), ex.getMessage()); } + /** + * The rejection stands, but its stated reason must not: HUMAN members ship (I6 + * — this same class validates them), so blaming their absence tells a config + * author a shipped feature is missing. What is actually absent is the resume + * path a paused tie-break would need. Asserting on the alternatives rather than + * on prose keeps this test from pinning the wording again. + */ @Test - void votePhase_humanDecides_isRejectedUntilI6() { + void votePhase_humanDecides_isRejectedPendingResumePath() { var ex = assertThrows(IllegalArgumentException.class, () -> AgentGroupStore.validateVotePhases(voteGroup( votePhase(TurnOrder.PARALLEL, ContextScope.NONE, new VoteConfig(VoteMethod.MAJORITY, OptionsSource.EXPLICIT, List.of("A", "B"), 0.5, Map.of(), false, TiePolicy.HUMAN_DECIDES))))); - assertTrue(ex.getMessage().contains("I6"), ex.getMessage()); + assertTrue(ex.getMessage().contains("HUMAN_DECIDES"), ex.getMessage()); + assertTrue(ex.getMessage().contains("MODERATOR_DECIDES"), ex.getMessage()); + assertTrue(ex.getMessage().contains("NO_DECISION"), ex.getMessage()); + assertFalse(ex.getMessage().contains("not available yet"), + "must not claim HUMAN members are unavailable — they ship: " + ex.getMessage()); } @Test diff --git a/src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java b/src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java new file mode 100644 index 000000000..00a89aee9 --- /dev/null +++ b/src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java @@ -0,0 +1,257 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.docs; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Set; +import java.util.TreeSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Keeps the documentation's internal links honest. + *

    + * A repository review found 38 broken relative links. They were not 38 separate + * mistakes: 32 came from one, when {@code planning/} moved to the repository + * root and every file in it kept computing {@code ../} and {@code ../../} as + * though it still lived under {@code docs/}. So {@code ../../AGENTS.md} pointed + * outside the repository entirely, and nothing noticed for as long as those + * files existed. The rest pointed into a {@code .gitbook/assets/} directory + * that does not exist in this repository at all, which broke the + * onboarding tutorials specifically — the pages a new user reads + * first. + *

    + * Link rot is invisible to every other check in this build: markdown compiles + * to nothing, so a wrong path is indistinguishable from a right one until a + * human clicks it. This test is the check. + */ +@DisplayName("documentation links") +class DocumentationLinksTest { + + /** + * Markdown inline links, e.g. {@code [text](target)}. The target group stops at + * the first {@code )}, which is why targets containing parentheses are excluded + * below rather than parsed. + */ + private static final Pattern LINK = Pattern.compile("]\\(([^)]+)\\)"); + + /** Fenced code blocks — their contents are examples, not links. */ + private static final Pattern FENCE = Pattern.compile("(?ms)^```.*?^```"); + + /** Inline code spans, for the same reason. */ + private static final Pattern CODE_SPAN = Pattern.compile("`[^`\\n]*`"); + + /** Directories with no documentation to check (or not ours to check). */ + private static final Set SKIPPED_DIRS = Set.of("target", ".git", "node_modules", ".claude", ".mvn"); + + private static Path repoRoot() { + // Surefire runs with the project basedir as the working directory. + Path root = Path.of("").toAbsolutePath(); + assertTrue(Files.isRegularFile(root.resolve("pom.xml")), + "expected the working directory to be the project root, was " + root); + return root; + } + + /** + * Collects every markdown file, pruning the directories in + * {@link #SKIPPED_DIRS} rather than walking them and filtering afterwards. + *

    + * The distinction is not cosmetic here. {@code target/} always exists when + * these tests run and holds tens of thousands of class files, {@code .git/} is + * comparably large, and this repository's own agent worktrees live under + * {@code .claude/} — so a filter-after-walk pays the full I/O cost of all three + * on every run, and would recurse through nested checkouts. + */ + private static List markdownFiles(Path root) { + List found = new ArrayList<>(); + try { + Files.walkFileTree(root, new SimpleFileVisitor<>() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) { + if (!dir.equals(root) && SKIPPED_DIRS.contains(dir.getFileName().toString())) { + return FileVisitResult.SKIP_SUBTREE; + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { + if (attrs.isRegularFile() + && file.getFileName().toString().toLowerCase(Locale.ROOT).endsWith(".md")) { + found.add(file); + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) { + return FileVisitResult.CONTINUE; // an unreadable entry is not this test's business + } + }); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return found; + } + + /** + * Strips fenced blocks and inline code so that documentation OF link syntax — + * {@code `![alt](uri)`} in the output-format tables, for instance — is not + * mistaken for a link TO something called "uri". + */ + private static String stripCode(String markdown) { + String withoutFences = FENCE.matcher(markdown).replaceAll(""); + return CODE_SPAN.matcher(withoutFences).replaceAll(""); + } + + private static boolean isExternalOrAnchor(String target) { + String t = target.trim(); + return t.isEmpty() + || t.startsWith("#") + || t.startsWith("http://") + || t.startsWith("https://") + || t.startsWith("mailto:") + || t.startsWith(" + * This matters more than it looks. Windows and macOS resolve + * {@code ../security.md} to {@code SECURITY.md}, so a link that is broken on + * the Linux CI runner — and for every reader of the published docs — looks + * perfectly fine to the developer who wrote it. That is exactly how + * {@code planning/agentic-improvements-plan.md} came to point at the security + * policy at the repository root while its own link text said + * {@code docs/security.md}. Without this check the test would have inherited + * the same blind spot as the human. + */ + private static String mismatchedCasing(Path resolved) { + try { + Path real = resolved.toRealPath(LinkOption.NOFOLLOW_LINKS); + String actual = real.getFileName().toString(); + String requested = resolved.getFileName().toString(); + return actual.equals(requested) ? null : actual; + } catch (IOException e) { + return null; // existence was already established; nothing more to say + } + } + + @Test + @DisplayName("every relative link in every tracked markdown file resolves to a real path") + void everyRelativeLinkResolves() { + Path root = repoRoot(); + List broken = new ArrayList<>(); + + for (Path file : markdownFiles(root)) { + String body; + try { + body = Files.readString(file, StandardCharsets.UTF_8); + } catch (IOException | RuntimeException e) { + continue; // unreadable/non-UTF-8 files are not this test's business + } + Matcher m = LINK.matcher(stripCode(body)); + while (m.find()) { + String target = m.group(1).trim(); + if (isExternalOrAnchor(target)) { + continue; + } + // Drop any '#fragment' and surrounding angle brackets before resolving. + String pathPart = target.replaceAll("^<|>$", ""); + int hash = pathPart.indexOf('#'); + if (hash >= 0) { + pathPart = pathPart.substring(0, hash); + } + if (pathPart.isBlank()) { + continue; + } + String decoded = URLDecoder.decode(pathPart, StandardCharsets.UTF_8); + // A leading '/' is repository-root-relative (how the forge renders + // it), NOT filesystem-absolute — resolving it against the file's + // own directory would send it to the drive root. + Path resolved = decoded.startsWith("/") + ? root.resolve(decoded.substring(1)).normalize() + : file.getParent().resolve(decoded).normalize(); + if (!resolved.startsWith(root)) { + // normalize() collapses '..' but does not confine the result to + // the repository. `../../thing.md` can land on a real file + // outside the checkout and quietly pass — which is the very + // mistake that put 32 planning/ links outside the repo. + broken.add(root.relativize(file) + " -> " + target + + " (escapes the repository root)"); + } else if (!Files.exists(resolved)) { + broken.add(root.relativize(file) + " -> " + target); + } else { + String actualCasing = mismatchedCasing(resolved); + if (actualCasing != null) { + broken.add(root.relativize(file) + " -> " + target + + " (case mismatch — the file on disk is '" + actualCasing + "')"); + } + } + } + } + + assertEquals(List.of(), broken, + "broken relative link(s) in documentation:\n " + String.join("\n ", broken)); + } + + /** + * {@code SUMMARY.md} is the published table of contents. A page missing from it + * still renders, still passes every other check, and is simply unreachable by + * navigation — which is how {@code security-review.md} and + * {@code release-notes-6.0.2.md} went unlisted. + */ + @Test + @DisplayName("every page under docs/ is reachable from SUMMARY.md") + void everyDocIsListedInSummary() throws IOException { + Path docs = repoRoot().resolve("docs"); + String summary = Files.readString(docs.resolve("SUMMARY.md"), StandardCharsets.UTF_8); + + Set missing = new TreeSet<>(); + // Recursive, not Files.list: the tutorials live in + // docs/creating-your-first-agent/ + // and docs/monitoring/, so a direct-children listing would let a nested + // page go unlisted — exactly the pages a newcomer needs to find. + for (Path p : markdownFiles(docs)) { + String name = p.getFileName().toString(); + if (name.equals("SUMMARY.md") || name.equals("README.md")) { + continue; // the index itself, and each folder's own landing page + } + String relative = docs.relativize(p).toString().replace('\\', '/'); + // SUMMARY.md links either by bare name or by path, depending on depth. + if (!summary.contains("(" + relative + ")") && !summary.contains("(" + name + ")") + && !summary.contains("/" + name + ")")) { + missing.add(relative); + } + } + + assertEquals(Set.of(), missing, + "these pages exist under docs/ but nothing in SUMMARY.md links to them, so they are " + + "unreachable in the published docs: " + missing); + } +} diff --git a/src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java b/src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java index fe434446e..c6c2872aa 100644 --- a/src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java +++ b/src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java @@ -7,6 +7,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -111,6 +112,234 @@ void flushPersists() { assertEquals(0, service.getQueueSize()); } + // ==================== sequence-table eviction ==================== + + /** + * The table used to be {@code clear()}ed on overflow, on the reasoning that + * re-seeding from {@code countByConversation} was "correct, only slower". It is + * not: an entry that is still queued has consumed a position the store cannot + * see yet, so the re-seed hands the same number out twice — and the verifier + * grades a duplicate as {@code BROKEN}, i.e. the ledger reporting the + * deployment as tampered because its own bookkeeping wrapped around. + */ + @Test + @DisplayName("sequence eviction — a conversation with queued entries is never re-seeded into a duplicate") + void sequenceEvictionKeepsQueuedConversationsUnique() { + when(auditStore.supportsSequence()).thenReturn(true); + when(auditStore.countByConversation(anyString())).thenReturn(0L); + service = createService(true, null); + + // Consume position 0 for "live" and leave it sitting in the queue. + service.submit(entry("live-1", "live", "agent1")); + + // Overflow the table. Nothing is flushed, so "live" is still queued when + // eviction runs — exactly the window the old clear() got wrong. + for (int i = 0; i < AuditLedgerService.MAX_TRACKED_CONVERSATIONS + 5; i++) { + service.submit(entry("fill-" + i, "filler-" + i, "agent1")); + } + + service.submit(entry("live-2", "live", "agent1")); + service.flush(); + + var persisted = ArgumentCaptor.forClass(List.class); + verify(auditStore).appendBatch(persisted.capture()); + @SuppressWarnings("unchecked") + List liveSequences = ((List) persisted.getValue()).stream() + .filter(e -> "live".equals(e.conversationId())) + .map(AuditEntry::sequence) + .toList(); + + assertEquals(List.of(0L, 1L), liveSequences, + "the second entry must continue the chain, not restart it at a position already taken"); + } + + /** + * The other half of the contract: once the queue has drained, those counters + * ARE safe to drop, so the table must actually shrink. Without this the fix + * could "pass" by simply never evicting, which would strand every later + * conversation on UNSEQUENCED. + */ + @Test + @DisplayName("sequence eviction — fully-persisted conversations are evicted so new ones still chain") + void sequenceEvictionReclaimsPersistedConversations() { + when(auditStore.supportsSequence()).thenReturn(true); + when(auditStore.countByConversation(anyString())).thenReturn(0L); + service = createService(true, null); + + for (int i = 0; i < AuditLedgerService.MAX_TRACKED_CONVERSATIONS + 5; i++) { + service.submit(entry("fill-" + i, "filler-" + i, "agent1")); + } + service.flush(); // everything is now durably in the store + + service.submit(entry("fresh-1", "fresh", "agent1")); + service.flush(); + + var persisted = ArgumentCaptor.forClass(List.class); + verify(auditStore, times(2)).appendBatch(persisted.capture()); + @SuppressWarnings("unchecked") + List lastBatch = (List) persisted.getValue(); + var fresh = lastBatch.stream().filter(e -> "fresh".equals(e.conversationId())).findFirst().orElseThrow(); + + assertEquals(0L, fresh.sequence(), + "with the queue drained the table must have room again, so this chains normally"); + assertNotEquals(AuditEntry.UNSEQUENCED, fresh.sequence()); + } + + /** + * The third place a consumed chain position can live. Once + * {@link AuditLedgerService#flush()} has polled a batch off the queue, those + * positions are in neither the queue nor the store until the append returns — + * so an eviction landing in that window would read their conversations as fully + * persisted and re-seed them straight into a duplicate. + *

    + * Driven from inside the mocked {@code appendBatch}, which is precisely when + * the batch is in flight. + */ + @Test + @DisplayName("sequence eviction — a conversation in the in-flight flush batch is not re-seeded") + void sequenceEvictionRetainsTheInFlightBatch() { + when(auditStore.supportsSequence()).thenReturn(true); + when(auditStore.countByConversation(anyString())).thenReturn(0L); + service = createService(true, null); + + // Position 0 for "live", then force it out of the queue and into the + // in-flight batch by flushing with an append that overflows the table + // while it is still executing. + service.submit(entry("live-1", "live", "agent1")); + doAnswer(invocation -> { + for (int i = 0; i < AuditLedgerService.MAX_TRACKED_CONVERSATIONS + 5; i++) { + service.submit(entry("fill-" + i, "filler-" + i, "agent1")); + } + return null; + }).when(auditStore).appendBatch(any()); + + service.flush(); + + // The append has returned, so "live" is genuinely persisted now; what + // matters is that eviction did not drop its counter mid-flight. + doAnswer(invocation -> null).when(auditStore).appendBatch(any()); + service.submit(entry("live-2", "live", "agent1")); + + var persisted = ArgumentCaptor.forClass(List.class); + service.flush(); + verify(auditStore, atLeastOnce()).appendBatch(persisted.capture()); + @SuppressWarnings("unchecked") + var live2 = ((List) persisted.getValue()).stream() + .filter(e -> "live".equals(e.conversationId())) + .findFirst().orElseThrow(); + + assertEquals(1L, live2.sequence(), + "the counter must have survived an eviction that ran while its entry was in the flush batch"); + } + + /** + * When every tracked conversation is genuinely live, a scan evicts nothing — + * and without a barrier the next submit for an unseen conversation would take + * the write lock and traverse the whole queue (bounded at 100k) to reach the + * same conclusion, with every other submitter blocked behind it on the read + * lock. Only a flush can change the answer, so only a flush lifts the barrier. + */ + @Test + @DisplayName("sequence eviction — a futile scan is not repeated until a flush could change the answer") + void futileEvictionScanIsNotRepeatedUntilFlush() { + when(auditStore.supportsSequence()).thenReturn(true); + when(auditStore.countByConversation(anyString())).thenReturn(0L); + service = createService(true, null); + + // Fill the table with conversations that are all still queued, so nothing + // is evictable. + for (int i = 0; i < AuditLedgerService.MAX_TRACKED_CONVERSATIONS + 5; i++) { + service.submit(entry("fill-" + i, "filler-" + i, "agent1")); + } + int trackedAfterFill = service.getTrackedConversationCount(); + + // Further unseen conversations must not each re-scan; they degrade to + // UNSEQUENCED, which is the honest verdict rather than a duplicate. + for (int i = 0; i < 50; i++) { + service.submit(entry("late-" + i, "late-conv-" + i, "agent1")); + } + assertEquals(trackedAfterFill, service.getTrackedConversationCount(), + "a barred scan must not grow the table either"); + assertEquals(1, service.getFutileEvictionScans(), + "the scan should have run once and then been barred, not once per submit"); + + // A flush moves entries out of the queue, so the next scan is worthwhile + // again — and this time it can actually evict. + service.flush(); + service.submit(entry("after-1", "after-flush", "agent1")); + + assertTrue(service.getTrackedConversationCount() < trackedAfterFill, + "once the queue drained, the barrier must lift and eviction reclaim the table"); + } + + /** + * A conversation with a dead-lettered position can never be re-seeded — the + * store count is smaller than the next free position once there is a gap, so + * re-seeding would hand the same numbers out twice. Those counters are + * therefore pinned for the process lifetime, which is only safe because they + * cannot fill the table: the undelivered cap counts sequences, so the + * worst case is one pinned conversation per tracked sequence. + *

    + * The failure this guards is a plausible future edit — raising + * {@code MAX_TRACKED_UNDELIVERED} to or past {@code MAX_TRACKED_CONVERSATIONS} + * — after which a long store outage could pin every slot and strand every later + * conversation on {@code UNSEQUENCED} until restart, with nothing failing to + * say so. + */ + @Test + @DisplayName("sequence eviction — pinned undelivered conversations do not block reclamation") + void undeliveredPinCannotExhaustTheTable() { + when(auditStore.supportsSequence()).thenReturn(true); + when(auditStore.countByConversation(anyString())).thenReturn(0L); + service = createService(true, null); + + // A store outage dead-letters this conversation, so position 0 is + // consumed but never persisted. Its counter is pinned from here on: the + // store count can no longer tell us where the chain resumes. + doThrow(new RuntimeException("db error")).when(auditStore).appendBatch(anyList()); + service.submit(entry("pinned-1", "pinned", "agent1")); + service.flush(); + service.flush(); + service.flush(); // dead-lettered + assertEquals(Set.of(0L), service.undeliveredSequences("pinned"), + "precondition: the outage left an unattributed position for this conversation"); + + // Store recovers. Fill the table with conversations that DO persist. + doAnswer(invocation -> null).when(auditStore).appendBatch(anyList()); + for (int i = 0; i < AuditLedgerService.MAX_TRACKED_CONVERSATIONS + 5; i++) { + service.submit(entry("fill-" + i, "filler-" + i, "agent1")); + } + service.flush(); + + // The pinned conversation is retained while the persisted ones are + // reclaimed, so a new conversation still gets a real chain position + // rather than being stranded on UNSEQUENCED. + service.submit(entry("fresh-1", "fresh", "agent1")); + service.flush(); + + var persisted = ArgumentCaptor.forClass(List.class); + verify(auditStore, atLeastOnce()).appendBatch(persisted.capture()); + @SuppressWarnings("unchecked") + var fresh = ((List) persisted.getValue()).stream() + .filter(e -> "fresh".equals(e.conversationId())) + .findFirst().orElseThrow(); + assertNotEquals(AuditEntry.UNSEQUENCED, fresh.sequence(), + "a pinned conversation must not cost later conversations their chain position"); + assertEquals(0L, fresh.sequence()); + + // And the pin did its job: the dead-lettered position is not handed out + // a second time. + service.submit(entry("pinned-2", "pinned", "agent1")); + service.flush(); + verify(auditStore, atLeastOnce()).appendBatch(persisted.capture()); + @SuppressWarnings("unchecked") + var pinnedSecond = ((List) persisted.getValue()).stream() + .filter(e -> "pinned".equals(e.conversationId())) + .findFirst().orElseThrow(); + assertEquals(1L, pinnedSecond.sequence(), + "the counter survived, so the chain resumes past the dead-lettered 0 rather than reusing it"); + } + @Test @DisplayName("flush — does nothing when queue is empty") void flushEmptyQueue() { @@ -256,7 +485,7 @@ void droppedEntryDoesNotConsumeAChainPosition() { svc.submit(entry("id-3", "conv-1", "agent-1")); // queued → must be sequence 1 svc.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore, times(2)).appendBatch(captor.capture()); List writtenSequences = ((List>) (List) captor.getAllValues()).stream().flatMap(List::stream) .map(AuditEntry::sequence).toList(); @@ -360,7 +589,7 @@ void entriesAreNumberedPerConversation() { service.submit(entry("id-3", "conv-b", "agent-1")); service.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore).appendBatch(captor.capture()); @SuppressWarnings("unchecked") List written = captor.getValue(); @@ -380,7 +609,7 @@ void sequenceIsSeededFromTheStore() { service.submit(entry("id-1", "conv-a", "agent-1")); service.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore).appendBatch(captor.capture()); @SuppressWarnings("unchecked") List written = captor.getValue(); @@ -400,7 +629,7 @@ void storeWithoutSequenceSupportGetsUnsequencedEntries() { service.submit(entry("id-1", "conv-a", "agent-1")); service.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore).appendBatch(captor.capture()); @SuppressWarnings("unchecked") List written = captor.getValue(); @@ -417,7 +646,7 @@ void signedEntryVerifies() { service.submit(entry("id-1", "conv-a", "agent-1")); service.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore).appendBatch(captor.capture()); @SuppressWarnings("unchecked") List written = captor.getValue(); @@ -442,7 +671,7 @@ void pseudonymisedStoredEntryStillVerifies() { service.submit(entry("id-1", "conv-a", "agent-1")); service.flush(); - var captor = org.mockito.ArgumentCaptor.forClass(List.class); + var captor = ArgumentCaptor.forClass(List.class); verify(auditStore).appendBatch(captor.capture()); @SuppressWarnings("unchecked") List written = captor.getValue(); diff --git a/src/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.java b/src/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.java new file mode 100644 index 000000000..e9ff96199 --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.java @@ -0,0 +1,120 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.httpclient; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.net.http.HttpRequest; +import java.time.Duration; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins {@link SafeHttpClient}'s per-hop timeout backstop. + *

    + * The class advertised an "overall wall-clock timeout enforced across all + * hops", but that budget is only consulted BETWEEN hops. A single hop that + * completes its handshake and then trickles — or never finishes — its response + * body therefore hung forever, because control never returned to the check. + * Redirect hops already carried a fallback; the initial request had whatever + * the caller set, which for a caller that set nothing was no bound at all. + *

    + * Deliberately separate from {@code SafeHttpClientTest}: that one binds a + * loopback HTTP server in {@code @BeforeEach}, so it cannot run in environments + * without loopback sockets. These cases are pure request-shaping and always + * run. + */ +@DisplayName("SafeHttpClient — per-hop timeout backstop") +class SafeHttpClientTimeoutTest { + + private static final URI TARGET = URI.create("https://example.test/resource"); + + @Nested + @DisplayName("withDefaultTimeout") + class WithDefaultTimeout { + + @Test + @DisplayName("a request with no timeout gets the default bound — the hang this fixes") + void unboundedRequestIsBounded() { + HttpRequest original = HttpRequest.newBuilder(TARGET).GET().build(); + assertTrue(original.timeout().isEmpty(), "precondition: the caller set no timeout"); + + HttpRequest bounded = SafeHttpClient.withDefaultTimeout(original); + + assertTrue(bounded.timeout().isPresent(), "an unbounded request must not reach the wire unbounded"); + assertEquals(Duration.ofSeconds(15), bounded.timeout().orElseThrow()); + } + + @Test + @DisplayName("a caller's own timeout is never overridden") + void callerTimeoutWins() { + Duration callerBound = Duration.ofSeconds(3); + HttpRequest original = HttpRequest.newBuilder(TARGET).timeout(callerBound).GET().build(); + + HttpRequest result = SafeHttpClient.withDefaultTimeout(original); + + assertSame(original, result, "no rebuild is needed when the caller already bounded the request"); + assertEquals(callerBound, result.timeout().orElseThrow()); + } + + @Test + @DisplayName("the rebuild preserves method, headers and body") + void rebuildPreservesTheRequest() { + HttpRequest original = HttpRequest.newBuilder(TARGET) + .header("Authorization", "Bearer token-value") + .header("X-Custom", "one") + .header("X-Custom", "two") + .POST(HttpRequest.BodyPublishers.ofString("payload")) + .build(); + + HttpRequest bounded = SafeHttpClient.withDefaultTimeout(original); + + assertEquals("POST", bounded.method(), "rebuilding must not silently downgrade the method"); + assertEquals(TARGET, bounded.uri()); + assertEquals("Bearer token-value", bounded.headers().firstValue("Authorization").orElse(null)); + assertEquals(List.of("one", "two"), bounded.headers().allValues("X-Custom"), + "a repeated header must keep every value, not just the last"); + assertTrue(bounded.bodyPublisher().isPresent(), "the body must survive the rebuild"); + assertEquals(original.bodyPublisher().orElseThrow().contentLength(), + bounded.bodyPublisher().orElseThrow().contentLength()); + } + + @Test + @DisplayName("HttpClient-managed headers are not copied — setting them explicitly is rejected") + void managedHeadersAreNotCopied() { + // Content-Length is set by HttpClient from the body publisher; copying + // it across would throw IllegalArgumentException on build(). + HttpRequest original = HttpRequest.newBuilder(TARGET) + .header("X-Kept", "yes") + .POST(HttpRequest.BodyPublishers.ofString("payload")) + .build(); + + HttpRequest bounded = SafeHttpClient.withDefaultTimeout(original); + + assertFalse(bounded.headers().firstValue("Content-Length").isPresent(), + "Content-Length is HttpClient's to set"); + assertEquals("yes", bounded.headers().firstValue("X-Kept").orElse(null)); + } + + @Test + @DisplayName("a GET with no body rebuilds without inventing one") + void bodylessRequestStaysBodyless() { + HttpRequest original = HttpRequest.newBuilder(TARGET).GET().build(); + + HttpRequest bounded = SafeHttpClient.withDefaultTimeout(original); + + assertEquals("GET", bounded.method()); + assertEquals(0L, bounded.bodyPublisher().map(HttpRequest.BodyPublisher::contentLength).orElse(0L), + "a GET must not gain a body just because it was rebuilt"); + } + } +} diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.java index 19529d1eb..afdf00f1f 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.java @@ -9,12 +9,16 @@ import org.junit.jupiter.api.Test; import java.lang.reflect.Method; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * Guards the one failure mode {@link McpToolFilter} makes silent: a tool that @@ -44,9 +48,9 @@ class McpToolFilterCoverageTest { *

    * Listed explicitly rather than classpath-scanned: a scan needs an indexing * dependency in a plain unit test, and the set of tool classes changes rarely - * and visibly. If you add a new {@code Mcp*Tools} class, add it here — the test - * below is what makes forgetting the whitelist loud, so this list is the one - * thing it cannot check for you. + * and visibly. If you add a new {@code Mcp*Tools} class, add it here — + * {@link #everyToolClassInThePackageIsListed()} fails until you do, so + * forgetting is loud rather than silent. */ private static final List> TOOL_CLASSES = List.of( McpAdminTools.class, McpConversationTools.class, McpSetupTools.class, @@ -99,4 +103,36 @@ void docsToolsAreExposed() { assertEquals(true, McpToolFilter.MCP_TOOLS.contains(name), name + " must be exposed to MCP clients"); } } + + /** + * Closes the one hole the tests above cannot see. Both of them start from + * {@link #TOOL_CLASSES}, so a brand-new {@code Mcp*Tools} class that nobody + * adds to that list has its tools invisible AND leaves every assertion green — + * the same silent failure this file exists to prevent, one level up. + *

    + * Counting the compiled classes in the package needs no indexing dependency: + * they are already on disk next to the ones being tested. + */ + @Test + @DisplayName("every Mcp*Tools class in the package is in TOOL_CLASSES — a new one cannot go unnoticed") + void everyToolClassInThePackageIsListed() throws Exception { + var codeSource = McpAdminTools.class.getProtectionDomain().getCodeSource().getLocation(); + Path packageDir = Path.of(codeSource.toURI()).resolve("ai/labs/eddi/engine/mcp"); + assertTrue(Files.isDirectory(packageDir), "cannot locate compiled package at " + packageDir); + + Set onDisk; + try (var entries = Files.list(packageDir)) { + onDisk = entries.map(p -> p.getFileName().toString()) + // Nested/anonymous classes carry a '$'; only top-level tool + // holders can declare the @Tool methods the SPI scans. + .filter(n -> n.startsWith("Mcp") && n.endsWith("Tools.class") && !n.contains("$")) + .map(n -> n.substring(0, n.length() - ".class".length())) + .collect(Collectors.toCollection(TreeSet::new)); + } + + var listed = TOOL_CLASSES.stream().map(Class::getSimpleName).collect(Collectors.toCollection(TreeSet::new)); + assertEquals(onDisk, listed, + "TOOL_CLASSES is out of step with the package. A class present on disk but not listed has its " + + "@Tool methods checked by nothing, so forgetting the whitelist stays silent: " + onDisk); + } } diff --git a/src/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java b/src/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java index 091fc5e19..1dd0ef8a7 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java @@ -396,4 +396,73 @@ void resetAllClearsTheIndex() { assertEquals(0, rateLimiter.scanCandidateCount("toolB")); } } + + /** + * A refill computed as {@code elapsedNanos * limit / WINDOW_NANOS} in long + * arithmetic overflows once a bucket has been idle long enough — about 107 days + * at the default limit of 1000, sooner at a higher one. The wrapped product is + * negative, so instead of refilling, the bucket's token count is driven below + * zero and {@code tryAcquire} refuses every subsequent call. + *

    + * The failure mode is the nasty kind: a rate limiter that silently latches shut + * and denies a tool forever, with no error and nothing in the logs to connect + * it to elapsed time. + */ + @Nested + @DisplayName("refill over a long idle window") + class LongIdleRefill { + + private static final long NANOS_PER_DAY = 86_400L * 1_000_000_000L; + + @Test + @DisplayName("a bucket idle past the long-overflow threshold refills instead of latching shut") + void idleBucketRefillsRatherThanLatchingShut() { + var bucket = new ToolRateLimiter.RateLimitBucket(1000); + + // Drain it, so a broken refill cannot be masked by a full bucket. + for (int i = 0; i < 1000; i++) { + assertTrue(bucket.tryAcquire(), "bucket should start with its full allowance"); + } + assertFalse(bucket.tryAcquire(), "drained bucket denies until it refills"); + + // 200 days idle: past the ~107-day point where elapsedNanos * 1000 + // exceeds Long.MAX_VALUE. + bucket.backdateLastRefill(200 * NANOS_PER_DAY); + + assertTrue(bucket.tryAcquire(), + "after a long idle window the bucket must be full again, not permanently denied"); + assertTrue(bucket.tokenCount() >= 0.0, + "token count must never go negative — that is the latched-shut state"); + } + + @Test + @DisplayName("the refill saturates at the limit rather than overshooting") + void longIdleRefillIsStillCappedAtLimit() { + var bucket = new ToolRateLimiter.RateLimitBucket(10); + assertTrue(bucket.tryAcquire()); + + bucket.backdateLastRefill(3650 * NANOS_PER_DAY); // ten years + + assertEquals(10, bucket.getRemaining(), + "an arbitrarily long idle period grants the limit, never more"); + } + + @Test + @DisplayName("a normal short idle window still refills proportionally") + void shortIdleRefillIsUnchanged() { + var bucket = new ToolRateLimiter.RateLimitBucket(60); + for (int i = 0; i < 60; i++) { + assertTrue(bucket.tryAcquire()); + } + assertFalse(bucket.tryAcquire()); + + // Half a window back should return roughly half the allowance; the + // guard against the overflow fix accidentally changing normal maths. + bucket.backdateLastRefill(30_000L * 1_000_000L); + + int remaining = bucket.getRemaining(); + assertTrue(remaining >= 25 && remaining <= 35, + "half a window should restore about half the allowance, got " + remaining); + } + } }