fix: audit ledger self-tamper defect, plus review fixes across CI, docs and hygiene - #677
Conversation
… docs Split out of #675 so automated review can actually run — that PR carried a 263-file mechanical refactor alongside these changes, and both CodeRabbit (>100 files) and Copilot (>300) declined to review it. The refactor follows in a stacked PR. The one that matters: AuditLedgerService capped its sequence table at 50,000 conversations and cleared the whole thing on overflow, reasoning that re-seeding from countByConversation was "correct, only slower". It is not. Entries sit in the queue for up to a flush interval, so a conversation with queued entries has consumed positions the store cannot see yet, and the re-seed hands the same position out twice. The verifier grades a duplicate exactly like a gap (BROKEN) and, unlike a gap, `undelivered` cannot exculpate it — so the ledger accused the deployment of deleting audit records because its own bookkeeping wrapped around. On a busy deployment the queue is never empty, so essentially every overflow produced them. Eviction now drops only counters whose positions are all accounted for. A ReentrantReadWriteLock spans "position consumed" to "entry visible in the queue" against eviction, and flush() publishes its in-flight batch under the same lock — writing the tests showed that publishing after the drain left a window where entries were in neither collection. Also: - AgentGroupStore rejected HUMAN_DECIDES as needing "human group members (I6), which are not available yet", 150 lines below its own I6 matrix that validates them. They shipped in 10c; the missing piece is the resume path. - SafeHttpClient claimed a wall-clock timeout across all hops, but the budget is only checked between hops, so a hop that trickles its body hung forever. - ToolRateLimiter.refill() overflowed after ~107 idle days, latching the bucket shut. - ConversationStepRunner registered the in-flight conversation outside the try whose finally unregisters it. - install.sh/install.ps1 (the README's `curl | bash` path) were in no CI path filter and had no lint at all. New shell-lint job. - auto-approve treated an absent check as passing; a conflicted PR that never triggers CI would have been approved as "all checks passed". - 38 broken doc links, 32 from one cause (planning/ computing ../ as if under docs/). Dead code and a committed 1.4 MB grafana.db removed; an orphaned richer dashboard rescued into docs/monitoring/ and wired up. Every fix has a mutation-checked regression test, plus two class-of-bug guards: DocumentationLinksTest (which caught a link that only resolved because Windows is case-insensitive) and the MCP tool-class drift check.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request updates repository automation, monitoring delivery, documentation validation, audit sequencing, HTTP timeout handling, cleanup behavior, random selection, rate limiting, and MCP tool coverage. ChangesRepository and CI automation
Monitoring
Documentation
Runtime reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of defects and “uncovered gap” issues found during a full-repo review, centered on preventing false audit-ledger tamper reports under load, tightening reliability/timeout behavior, and adding CI + test coverage to areas that previously had none (installers, link rot, MCP tool exposure drift).
Changes:
- Fixes
AuditLedgerServicesequence-counter eviction/flush concurrency to prevent duplicate sequence assignment and falseChainStatus.BROKENunder load, with regression tests. - Hardens request-path behavior: per-hop timeout backstop in
SafeHttpClient, long-idle overflow fix inToolRateLimiter, and safer in-flight conversation registration. - Closes “outside the build” gaps: adds shell linting in CI for installers/hooks, strengthens auto-approve gating, repairs docs links/ToC coverage, and rescues/mounts an orphaned Grafana dashboard.
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java | Adds regression coverage for long-idle rate limiter overflow / refill behavior. |
| src/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.java | Adds coverage to detect new Mcp*Tools classes not being added to the allowlist. |
| src/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.java | Adds pure request-shaping tests for SafeHttpClient timeout backstop without requiring loopback servers. |
| src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java | Adds regression tests for sequence-table eviction correctness across queued and in-flight entries. |
| src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java | Adds repo-wide markdown link and docs ToC reachability checks. |
| src/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.java | Updates test expectations to match corrected HUMAN_DECIDES rejection rationale. |
| src/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.java | Switches random selection to ThreadLocalRandom for lower contention/allocation on hot path. |
| src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java | Fixes long-idle overflow in refill math; adds a test seam for idle-window simulation. |
| src/main/java/ai/labs/eddi/engine/memory/model/Data.java | Replaces per-instance Random usage with ThreadLocalRandom. |
| src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java | Removes unused exception class (dead code). |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java | Switches random deployment selection to ThreadLocalRandom on request path. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.java | Moves in-flight registration-dependent setup inside try to avoid stranding entries on Error. |
| src/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.java | Applies per-hop default request timeout when caller sets none; updates docs and redirect-hop timeout handling. |
| src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java | Implements safe sequence-counter eviction + in-flight batch visibility and locking to prevent duplicate sequencing. |
| src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java | Removes unimplemented JAX-RS interface that advertised endpoints without serving them. |
| src/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.java | Corrects HUMAN_DECIDES rejection message to reflect missing resume path (not missing human members). |
| planning/security-hardening-remaining.md | Fixes broken prerequisite links and references after directory layout changes. |
| planning/observability-and-pipeline-plan.md | Fixes docs and source links (including moved LifecycleManager path). |
| planning/multi-agent-ux-improvements.md | Fixes docs and source references to correct paths. |
| planning/memory-architecture-plan.md | Fixes Nine Pillars link to correct docs location. |
| planning/documentation-updates-plan.md | Fixes links (including .env.example path) to resolve correctly from planning root. |
| planning/agentic-improvements-plan.md | Fixes extensive internal references after planning files moved to repo root. |
| install.sh | Ensures monitoring dashboard file list includes rescued operations dashboard. |
| install.ps1 | Ensures monitoring dashboard file list includes rescued operations dashboard. |
| grafana-data/provisioning/datasources/prometheus.yml | Removes orphaned legacy Grafana provisioning (bind-mount era). |
| grafana-data/provisioning/dashboards/dashboard.yml | Removes orphaned legacy Grafana provisioning (bind-mount era). |
| docs/SUMMARY.md | Adds missing pages to published docs table of contents. |
| docs/monitoring/eddi-operations-dashboard.json | Adds rescued “Operations Command Center” dashboard JSON under docs provisioning. |
| docs/httpcalls.md | Replaces broken Postman collection link with OpenAPI-import instructions. |
| docs/creating-your-first-agent/README.md | Replaces broken asset image with an inline Mermaid diagram explaining config/pipeline flow. |
| docs/creating-your-first-agent/creating-your-first-agent.md | Replaces broken Postman collection link with OpenAPI-import instructions. |
| docs/creating-your-first-agent/creating-your-first-agent-1.md | Replaces broken Postman collection link with OpenAPI-import instructions. |
| docs/conversations.md | Replaces broken sample agent collection link with OpenAPI-import instructions. |
| docs/changelog.md | Documents the review findings, fixes, and added regression coverage. |
| docker-compose.monitoring.yml | Mounts the rescued operations dashboard into Grafana dashboards directory. |
| .gitignore | Ignores grafana-data/ runtime state to prevent recommitting Grafana DB/files. |
| .github/workflows/ci.yml | Adds scripts path filter output and a shell-lint job to verify installers/hooks. |
| .github/workflows/auto-approve-copilot.yml | Requires gating check runs to be present by name before auto-approving. |
| .github/PULL_REQUEST_TEMPLATE.md | Fixes template links to CONTRIBUTING sections via absolute GitHub URLs. |
| .gitattributes | Forces LF for .githooks/** to prevent broken execution on Unix-like platforms. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private static List<Path> markdownFiles(Path root) { | ||
| try (Stream<Path> paths = Files.walk(root)) { | ||
| return paths.filter(Files::isRegularFile) | ||
| .filter(p -> p.getFileName().toString().toLowerCase(Locale.ROOT).endsWith(".md")) | ||
| .filter(p -> { | ||
| for (Path part : root.relativize(p)) { | ||
| if (SKIPPED_DIRS.contains(part.toString())) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }) | ||
| .toList(); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| } |
Addresses Copilot's review comment on #677. markdownFiles() used Files.walk(root) and filtered SKIPPED_DIRS afterwards, so it paid the full traversal cost of every directory it then discarded. That is not cosmetic here: target/ always exists when tests run and holds tens of thousands of class files, .git/ is comparably large, and this repository's own agent worktrees live under .claude/ — so the walk also recursed through nested checkouts of the repository itself. Files.walkFileTree with SKIP_SUBTREE prunes those roots instead. The test drops from ~1.9-4.4s to ~0.6s, and detection is unchanged: reverting a link fix still fails it with the file and target named.
|
Addressed in 6714f89 — good catch, and it was measurable.
Now |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java (1)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the tracking bound injectable to shorten these tests.
Each of the three tests loops
MAX_TRACKED_CONVERSATIONS + 5times, so the class creates about 150,000AuditEntryinstances. The behaviour under test does not depend on the exact bound. IfcreateForTestingaccepted amaxTrackedConversationsvalue, the same assertions would hold with a bound of about 5 and the tests would run in milliseconds.The assertions themselves are precise and do catch the old
clear()behaviour, so this is a runtime concern only.Also applies to: 169-171, 210-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java` around lines 137 - 139, Allow AuditLedgerService.createForTesting to accept an injectable maxTrackedConversations value, and update the affected tests to use a small bound such as 5. Replace each MAX_TRACKED_CONVERSATIONS + 5 loop bound with the injected test bound plus the required overflow entries, while preserving the existing assertions and production default behavior.src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java (1)
402-412: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRate-limit the full-table warning. Each submission for an untracked conversation logs the warning while the sequence table remains full.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java` around lines 402 - 412, Rate-limit the warning in the full-table branch of the conversation sequence tracking logic so repeated submissions for untracked conversations do not log on every request while the table remains full. Preserve the existing AuditEntry.UNSEQUENCED return behavior and ensure warning state resets once capacity becomes available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 105-143: Update the PowerShell syntax check and PSScriptAnalyzer
steps to discover and validate all PowerShell files under scripts/ in addition
to install.ps1. Reuse the existing parser and analyzer logic for each discovered
file, while preserving the current error reporting and failure behavior.
In `@docs/changelog.md`:
- Around line 25-40: Update the opening claim in the changelog to match the
documented mutation-tested fixes: either add entries for the omitted HTTP
timeout handling, conversation cleanup, random selection, installer CI, and
dashboard delivery changes, or narrow the claim so it applies only to the fixes
listed in the table.
In `@docs/conversations.md`:
- Around line 644-648: Update the OpenAPI import instructions in
docs/conversations.md lines 644-648 and
docs/creating-your-first-agent/creating-your-first-agent-1.md lines 613-617 to
avoid presenting localhost as the universal host; use the deployment-specific
EDDI service host, or explicitly label the localhost URL as local-only in both
guides.
In `@docs/monitoring/eddi-operations-dashboard.json`:
- Line 47: Update the Prometheus expression in the “Top 10 Slowest Endpoints”
panel to calculate weighted latency by separately summing
http_server_requests_seconds_sum and http_server_requests_seconds_count by uri
and method, then divide the aggregated rates before applying topk(10). Preserve
the existing dashboard grouping and time window.
- Line 37: Update the Cost / hr stat panel target in the dashboard JSON to use a
distinct, non-resettable cost counter metric rather than eddi_tool_costs_total.
Rename the conflicting metric at its emission or mapping site, then query the
renamed counter with 3600 * sum(rate(...[1h])) while preserving the existing
panel configuration.
In `@install.ps1`:
- Around line 538-539: Update the monitoring download failure handling in
install.ps1 (lines 538-539) and install.sh (lines 622-623) to remove any
partially downloaded target file and exit with failure instead of warning and
continuing. Apply the same behavior to both installer paths for the required
Grafana and Prometheus monitoring files.
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 350-378: Throttle repeated failed eviction scans in
evictSequenceCountersIfFull by recording when no conversation was evicted and
returning early for subsequent unseen conversation IDs until state may have
changed, such as after a flush or a short cooldown. Preserve eviction when
eligible entries exist, and ensure the throttle state is updated or cleared when
eviction succeeds or relevant queue/in-flight state changes.
In `@src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java`:
- Around line 197-200: Update the link-target validation around resolved in
DocumentationLinksTest so normalized paths are accepted only when they remain
under root; reject targets that escape the repository before checking
Files.exists(resolved). Preserve the existing handling for root-relative and
file-relative links.
- Around line 229-235: Update the documentation page traversal in
DocumentationLinksTest to walk docs recursively rather than using
Files.list(docs), so Markdown files in nested directories such as
docs/creating-your-first-agent/ are included in SUMMARY.md coverage checks.
Preserve the existing exclusions for SUMMARY.md and README.md and the current
link-matching validation.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 402-412: Rate-limit the warning in the full-table branch of the
conversation sequence tracking logic so repeated submissions for untracked
conversations do not log on every request while the table remains full. Preserve
the existing AuditEntry.UNSEQUENCED return behavior and ensure warning state
resets once capacity becomes available.
In `@src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java`:
- Around line 137-139: Allow AuditLedgerService.createForTesting to accept an
injectable maxTrackedConversations value, and update the affected tests to use a
small bound such as 5. Replace each MAX_TRACKED_CONVERSATIONS + 5 loop bound
with the injected test bound plus the required overflow entries, while
preserving the existing assertions and production default behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9eb99b3-4e63-4c93-9887-c4db66df6869
⛔ Files ignored due to path filters (1)
grafana-data/grafana.dbis excluded by!**/*.db
📒 Files selected for processing (40)
.gitattributes.github/PULL_REQUEST_TEMPLATE.md.github/workflows/auto-approve-copilot.yml.github/workflows/ci.yml.gitignoredocker-compose.monitoring.ymldocs/SUMMARY.mddocs/changelog.mddocs/conversations.mddocs/creating-your-first-agent/README.mddocs/creating-your-first-agent/creating-your-first-agent-1.mddocs/creating-your-first-agent/creating-your-first-agent.mddocs/httpcalls.mddocs/monitoring/eddi-operations-dashboard.jsongrafana-data/provisioning/dashboards/dashboard.ymlgrafana-data/provisioning/datasources/prometheus.ymlinstall.ps1install.shplanning/agentic-improvements-plan.mdplanning/documentation-updates-plan.mdplanning/memory-architecture-plan.mdplanning/multi-agent-ux-improvements.mdplanning/observability-and-pipeline-plan.mdplanning/security-hardening-remaining.mdsrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.javasrc/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.javasrc/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.javasrc/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.javasrc/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.javasrc/main/java/ai/labs/eddi/engine/memory/model/Data.javasrc/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.javasrc/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.javasrc/test/java/ai/labs/eddi/docs/DocumentationLinksTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.javasrc/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java
💤 Files with no reviewable changes (4)
- grafana-data/provisioning/dashboards/dashboard.yml
- src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java
- grafana-data/provisioning/datasources/prometheus.yml
- src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java (1)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the tracking bound injectable to shorten these tests.
Each of the three tests loops
MAX_TRACKED_CONVERSATIONS + 5times, so the class creates about 150,000AuditEntryinstances. The behaviour under test does not depend on the exact bound. IfcreateForTestingaccepted amaxTrackedConversationsvalue, the same assertions would hold with a bound of about 5 and the tests would run in milliseconds.The assertions themselves are precise and do catch the old
clear()behaviour, so this is a runtime concern only.Also applies to: 169-171, 210-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java` around lines 137 - 139, Allow AuditLedgerService.createForTesting to accept an injectable maxTrackedConversations value, and update the affected tests to use a small bound such as 5. Replace each MAX_TRACKED_CONVERSATIONS + 5 loop bound with the injected test bound plus the required overflow entries, while preserving the existing assertions and production default behavior.src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java (1)
402-412: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRate-limit the full-table warning. Each submission for an untracked conversation logs the warning while the sequence table remains full.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java` around lines 402 - 412, Rate-limit the warning in the full-table branch of the conversation sequence tracking logic so repeated submissions for untracked conversations do not log on every request while the table remains full. Preserve the existing AuditEntry.UNSEQUENCED return behavior and ensure warning state resets once capacity becomes available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 105-143: Update the PowerShell syntax check and PSScriptAnalyzer
steps to discover and validate all PowerShell files under scripts/ in addition
to install.ps1. Reuse the existing parser and analyzer logic for each discovered
file, while preserving the current error reporting and failure behavior.
In `@docs/changelog.md`:
- Around line 25-40: Update the opening claim in the changelog to match the
documented mutation-tested fixes: either add entries for the omitted HTTP
timeout handling, conversation cleanup, random selection, installer CI, and
dashboard delivery changes, or narrow the claim so it applies only to the fixes
listed in the table.
In `@docs/conversations.md`:
- Around line 644-648: Update the OpenAPI import instructions in
docs/conversations.md lines 644-648 and
docs/creating-your-first-agent/creating-your-first-agent-1.md lines 613-617 to
avoid presenting localhost as the universal host; use the deployment-specific
EDDI service host, or explicitly label the localhost URL as local-only in both
guides.
In `@docs/monitoring/eddi-operations-dashboard.json`:
- Line 47: Update the Prometheus expression in the “Top 10 Slowest Endpoints”
panel to calculate weighted latency by separately summing
http_server_requests_seconds_sum and http_server_requests_seconds_count by uri
and method, then divide the aggregated rates before applying topk(10). Preserve
the existing dashboard grouping and time window.
- Line 37: Update the Cost / hr stat panel target in the dashboard JSON to use a
distinct, non-resettable cost counter metric rather than eddi_tool_costs_total.
Rename the conflicting metric at its emission or mapping site, then query the
renamed counter with 3600 * sum(rate(...[1h])) while preserving the existing
panel configuration.
In `@install.ps1`:
- Around line 538-539: Update the monitoring download failure handling in
install.ps1 (lines 538-539) and install.sh (lines 622-623) to remove any
partially downloaded target file and exit with failure instead of warning and
continuing. Apply the same behavior to both installer paths for the required
Grafana and Prometheus monitoring files.
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 350-378: Throttle repeated failed eviction scans in
evictSequenceCountersIfFull by recording when no conversation was evicted and
returning early for subsequent unseen conversation IDs until state may have
changed, such as after a flush or a short cooldown. Preserve eviction when
eligible entries exist, and ensure the throttle state is updated or cleared when
eviction succeeds or relevant queue/in-flight state changes.
In `@src/test/java/ai/labs/eddi/docs/DocumentationLinksTest.java`:
- Around line 197-200: Update the link-target validation around resolved in
DocumentationLinksTest so normalized paths are accepted only when they remain
under root; reject targets that escape the repository before checking
Files.exists(resolved). Preserve the existing handling for root-relative and
file-relative links.
- Around line 229-235: Update the documentation page traversal in
DocumentationLinksTest to walk docs recursively rather than using
Files.list(docs), so Markdown files in nested directories such as
docs/creating-your-first-agent/ are included in SUMMARY.md coverage checks.
Preserve the existing exclusions for SUMMARY.md and README.md and the current
link-matching validation.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 402-412: Rate-limit the warning in the full-table branch of the
conversation sequence tracking logic so repeated submissions for untracked
conversations do not log on every request while the table remains full. Preserve
the existing AuditEntry.UNSEQUENCED return behavior and ensure warning state
resets once capacity becomes available.
In `@src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java`:
- Around line 137-139: Allow AuditLedgerService.createForTesting to accept an
injectable maxTrackedConversations value, and update the affected tests to use a
small bound such as 5. Replace each MAX_TRACKED_CONVERSATIONS + 5 loop bound
with the injected test bound plus the required overflow entries, while
preserving the existing assertions and production default behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9eb99b3-4e63-4c93-9887-c4db66df6869
⛔ Files ignored due to path filters (1)
grafana-data/grafana.dbis excluded by!**/*.db
📒 Files selected for processing (40)
.gitattributes.github/PULL_REQUEST_TEMPLATE.md.github/workflows/auto-approve-copilot.yml.github/workflows/ci.yml.gitignoredocker-compose.monitoring.ymldocs/SUMMARY.mddocs/changelog.mddocs/conversations.mddocs/creating-your-first-agent/README.mddocs/creating-your-first-agent/creating-your-first-agent-1.mddocs/creating-your-first-agent/creating-your-first-agent.mddocs/httpcalls.mddocs/monitoring/eddi-operations-dashboard.jsongrafana-data/provisioning/dashboards/dashboard.ymlgrafana-data/provisioning/datasources/prometheus.ymlinstall.ps1install.shplanning/agentic-improvements-plan.mdplanning/documentation-updates-plan.mdplanning/memory-architecture-plan.mdplanning/multi-agent-ux-improvements.mdplanning/observability-and-pipeline-plan.mdplanning/security-hardening-remaining.mdsrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.javasrc/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.javasrc/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.javasrc/main/java/ai/labs/eddi/engine/httpclient/SafeHttpClient.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationStepRunner.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.javasrc/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.javasrc/main/java/ai/labs/eddi/engine/memory/model/Data.javasrc/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.javasrc/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStoreTest.javasrc/test/java/ai/labs/eddi/docs/DocumentationLinksTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.javasrc/test/java/ai/labs/eddi/engine/httpclient/SafeHttpClientTimeoutTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiterTest.java
💤 Files with no reviewable changes (4)
- grafana-data/provisioning/dashboards/dashboard.yml
- src/main/java/ai/labs/eddi/engine/api/ILogoutEndpoint.java
- grafana-data/provisioning/datasources/prometheus.yml
- src/main/java/ai/labs/eddi/engine/lifecycle/exceptions/CannotExecuteException.java
🛑 Comments failed to post (2)
docs/monitoring/eddi-operations-dashboard.json (2)
37-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Prometheus documentation: What unit doesrate()return, and how should a counter that represents USD be converted to USD per hour?💡 Result:
The Prometheus
rate()function returns the per-second average rate of increase of a time series over a specified time range [1][2][3]. Because it divides the total increase in the counter by the number of seconds in the lookback window, the resulting unit is [original_unit]/second [1][4][5]. To convert a counter representing USD (e.g.,total_usd_earned) into a rate of "USD per hour," you must multiply the result of therate()function by 3,600 (the number of seconds in an hour) [3][5]. The recommended PromQL expression is: rate(total_usd_earned[5m]) * 3600 Alternatively, if you want the total increase over a specific hour rather than a per-second average scaled to an hour, you can use theincrease()function, which returns the total increase over the lookback window [3][5]: increase(total_usd_earned[1h]) Note:rate()andincrease()are specifically designed for counter metrics and automatically handle counter resets (e.g., when a service restarts) [1][2]. They should not be used with gauge metrics [1][6]. When usingrate(), be aware that it extrapolates values to the edges of the time range to account for scrape intervals, which provides a more accurate estimate but can introduce minor discrepancies over very short time ranges [1][7].Citations:
- 1: https://prometheus.io/docs/prometheus/latest/querying/functions/
- 2: https://prometheus.io/docs/prometheus/3.13/querying/functions/
- 3: https://last9.io/blog/prometheus-rate-function/
- 4: https://www.metricfire.com/blog/understanding-the-prometheus-rate-function/
- 5: https://stackoverflow.com/questions/65400298/promql-what-is-rate-function-meant-for
- 6: https://prometheus.io/docs/prometheus/latest/querying/basics/
- 7: https://stackguides.com/questions/58178702/expressing-revenue-metrics-with-prometheus
🏁 Script executed:
#!/bin/bash set -eu file="$(git ls-files 'docs/monitoring/eddi-operations-dashboard.json' | head -n 1)" printf '%s\n' "FILE=$file" wc -l "$file" cat -n "$file" | sed -n '28,55p' printf '\nMetric references and panel titles:\n' rg -n -C 2 'eddi_tool_costs_total|Cost / hr|cost' --glob '!node_modules' .Repository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash set -eu file='docs/monitoring/eddi-operations-dashboard.json' printf '%s\n' 'Dashboard panels containing cost metrics:' python3 - <<'PY' import json from pathlib import Path p = Path("docs/monitoring/eddi-operations-dashboard.json") data = json.loads(p.read_text()) for panel in data.get("panels", []): text = json.dumps(panel, separators=(",", ":")) if "eddi_tool_costs_total" in text or "Cost / hr" in text: print(json.dumps({ "id": panel.get("id"), "title": panel.get("title"), "type": panel.get("type"), "targets": panel.get("targets"), "fieldConfig": panel.get("fieldConfig"), }, indent=2)) PY printf '\nMetric definitions and related PromQL in tracked source files:\n' rg -n --no-heading --glob '*.java' --glob '*.yml' --glob '*.yaml' --glob '*.json' \ 'eddi_tool_costs_total|tool_costs_total|costs_total' . \ | grep -vE '(^\./(frontend|web|node_modules|dist|build)/|locales|i18n)' \ | head -n 200Repository: labsai/EDDI
Length of output: 3340
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' 'Tracked files that define or reference the metric:' git grep -l -I 'eddi_tool_costs_total' -- ':!docs/monitoring/eddi-operations-dashboard.json' \ | head -n 100 printf '\nRelevant source excerpts (excluding generated frontend assets):\n' while IFS= read -r f; do case "$f" in frontend/*|web/*|*/dist/*|*/build/*|*.min.js|*.map) continue ;; esac printf '\n--- %s ---\n' "$f" git grep -n -I -C 3 'eddi_tool_costs_total' -- "$f" | head -n 80 done < <(git grep -l -I 'eddi_tool_costs_total' -- ':!docs/monitoring/eddi-operations-dashboard.json')Repository: labsai/EDDI
Length of output: 334
🏁 Script executed:
#!/bin/bash set -eu file='docs/metrics.md' printf '%s\n' "Metric documentation excerpts:" rg -n -C 8 'eddi_tool_costs_total|tool_costs_total|cost' "$file" | head -n 160Repository: labsai/EDDI
Length of output: 7139
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' 'Cost metric instrumentation candidates:' rg -n --no-heading --glob '*.java' --glob '*.kt' --glob '*.ts' --glob '*.tsx' --glob '*.js' \ 'tool_costs|costs_total|toolCost|costTracking|MeterRegistry|Gauge\.builder|Counter\.builder' . \ | grep -vE '(^\./(frontend|web|node_modules|dist|build)/|/target/|\.min\.js)' \ | head -n 300Repository: labsai/EDDI
Length of output: 43313
🏁 Script executed:
#!/bin/bash set -eu file='src/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.java' printf '%s\n' 'ToolCostTracker metric registration and updates:' cat -n "$file" | sed -n '1,230p' cat -n "$file" | sed -n '300,345p'Repository: labsai/EDDI
Length of output: 11237
Use a distinct counter metric for the hourly cost panel.
eddi_tool_costs_totalis emitted from a resettable gauge, sorate()does not produce a valid cost rate. The per-tooleddi.tool.costscounter also maps to the same Prometheus name. Rename one metric, then query the counter with3600 * sum(rate(...[1h])).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/monitoring/eddi-operations-dashboard.json` at line 37, Update the Cost / hr stat panel target in the dashboard JSON to use a distinct, non-resettable cost counter metric rather than eddi_tool_costs_total. Rename the conflicting metric at its emission or mapping site, then query the renamed counter with 3600 * sum(rate(...[1h])) while preserving the existing panel configuration.
47-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Prometheus documentation: How should a weighted average be calculated from_sumand_countcounter series with multiple labels?💡 Result:
To calculate a weighted average (or the mean observation size) from Prometheus
_sumand_countcounter series, you must calculate the rate of increase for both metrics over a specified lookbehind window and then divide the two resulting vectors [1][2][3]. The standard PromQL expression to achieve this is: rate(metric_name_sum[5m]) / rate(metric_name_count[5m]) Key Principles for Aggregation: 1. Rate, then aggregate: Always calculate therate()of the counter series first [4]. Aggregating (e.g., usingsum()oravg()) before calculating the rate will produce incorrect results because it hides counter resets and breaks the rate calculation [4]. 2. Aggregating across labels: When you need to aggregate across multiple labels (e.g., calculating the average across all instances), aggregate the numerator and denominator separately before dividing [5][2]. This ensures the ratio remains mathematically valid [2]. Example for Aggregating by Job: sum by (job) (rate(metric_name_sum[5m])) / sum by (job) (rate(metric_name_count[5m])) This approach is statistically sound for standard summaries and classic histograms [1][2]. Do not attempt to take the "average of a ratio" or "average of an average," as these are not statistically valid operations [5][2]. If the metrics include extra labels that you want to ignore during the division to ensure proper vector matching, you can use theignoring()modifier [6] or simply ensure your aggregation (likesum by (...)) reduces the label sets of both the numerator and denominator to match [7][2].Citations:
- 1: https://github.com/prometheus/docs/blob/main/docs/practices/histograms.md
- 2: https://prometheus.io/docs/practices/rules/
- 3: https://stackoverflow.com/questions/65400298/promql-what-is-rate-function-meant-for
- 4: https://github.com/prometheus/docs/blob/main/docs/practices/the_zen.md
- 5: https://github.com/prometheus/docs/blob/main/docs/practices/rules.md
- 6: https://stackoverflow.com/questions/40666785/how-to-generate-an-average-in-prometheus
- 7: https://github.com/prometheus/prometheus/blob/f37442d2/docs/querying/examples.md
🏁 Script executed:
#!/bin/bash set -euo pipefail file="docs/monitoring/eddi-operations-dashboard.json" printf '%s\n' '--- dashboard context ---' sed -n '35,52p' "$file" printf '%s\n' '--- metric references ---' rg -n -C 3 'http_server_requests_seconds_(sum|count)|Top 10 Slowest Endpoints' . \ -g '*.json' -g '*.yaml' -g '*.yml' -g '*.md' -g '*.java' -g '*.kt' -g '*.ts' -g '*.js' \ || true printf '%s\n' '--- JSON validity ---' python3 - <<'PY' import json from pathlib import Path p = Path("docs/monitoring/eddi-operations-dashboard.json") data = json.loads(p.read_text()) matches = [] def walk(value): if isinstance(value, dict): if value.get("title") == "Top 10 Slowest Endpoints": matches.append(value) for child in value.values(): walk(child) elif isinstance(value, list): for child in value: walk(child) walk(data) for panel in matches: expr = panel["targets"][0]["expr"] print(expr) print("target_count:", len(panel["targets"])) print("matching_panels:", len(matches)) PYRepository: labsai/EDDI
Length of output: 16025
Calculate weighted endpoint latency.
Aggregate the duration and request-count rates separately before division. The current query sums per-series averages and can inflate latency when an endpoint has multiple status or outcome series.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/monitoring/eddi-operations-dashboard.json` at line 47, Update the Prometheus expression in the “Top 10 Slowest Endpoints” panel to calculate weighted latency by separately summing http_server_requests_seconds_sum and http_server_requests_seconds_count by uri and method, then divide the aggregated rates before applying topk(10). Preserve the existing dashboard grouping and time window.
… doc-test gaps All seven findings on #677, two of them real defects rather than polish. Eviction rescans could stall the submit path (Major). With every tracked conversation live, a scan evicts nothing — and 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 a barrier keyed on a flush generation counter now bars the rescan until one happens. Mutation-checked: removing the barrier turns 1 futile scan into 55. Monitoring downloads failed soft, but every one of those files 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. Now fatal in both installers, matching the reasoning already written for the Keycloak realm. DocumentationLinksTest had two holes: - normalize() collapses '..' but does not confine the result to the repo, so `../../thing.md` could land on a real file outside the checkout and pass — which is the exact mistake that put 32 planning/ links outside the repo. - The SUMMARY.md check used Files.list, which sees only direct children. Made recursive, and it immediately found docs/templates/baa-template.md unreachable from the table of contents. Now listed. Also: - Copilot: markdownFiles() walked target/, .git/ and .claude/ before filtering them. Pruned with SKIP_SUBTREE; the test drops from ~1.9-4.4s to ~0.6s. - scripts/preflight-local.ps1 was as unlinted as the installers were; the PowerShell parse and PSScriptAnalyzer steps now cover scripts/ too. - The OpenAPI import instructions hard-coded localhost, which only works when Postman runs on the EDDI host. - The changelog claimed every fix had a mutation-checked test. Several do not, for stated reasons; the claim is narrowed and the exceptions spelled out rather than left to imply more than was done.
|
All seven addressed in 313f382. Two were real defects, not polish — thank you. Eviction rescans could stall the submit path (the Major one). You were right that a full table of live conversations makes every subsequent submit for an unseen conversation take the write lock and traverse the whole queue to reach the same conclusion, with all other submitters blocked behind it. Only a flush can change the answer, so there is now a barrier keyed on a flush-generation counter. Mutation-checked: removing it turns 1 futile scan into 55 in the new On Monitoring downloads failing soft — agreed, and worse than it looks: each of those files is bind-mounted as a file, so a missing one makes Docker create a directory at the mount path and Grafana then fails to provision, including the dashboards that did download. Now fatal in both installers, which matches the reasoning already written a few lines below for the Keycloak realm. Both
The changelog claim — fair. Several fixes are not mutation-checked (SafeHttpClient's absent-bound, the Also fixed: Full suite re-run: unchanged from baseline (the only failures are this sandbox's loopback sockets, which CI covers). |
The shell-lint job I added in this branch failed on its own first real run —
which is the job working, just at my expense. Extending it to cover scripts/
passed `@('./install.ps1', './scripts')` to -Path, and that parameter is a
single String:
Cannot convert 'System.Object[]' to the type 'System.String' required by
parameter 'Path'.
Loop over the targets instead. Also skip Install-Module when the runner
already ships PSScriptAnalyzer, which it does — installing over it only emits
a "version is currently in use" warning.
Both PowerShell steps were run locally under pwsh this time rather than
reasoned about: the parse step passes for install.ps1 and
scripts/preflight-local.ps1, and the analyzer reports 9 warnings and 0 errors,
so the gate passes while still surfacing them in the log.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java:392
- Conversations with any dead-lettered position are added to
retain, butundeliveredis never cleared. After one store outage affects 50,000 distinct conversations, these counters permanently fill the sequence table; even after the queue drains and the store recovers, every new conversation remainsUNSEQUENCEDuntil restart. Reseeding needs to account for known undelivered positions so those counters can eventually be evicted, or the retained set needs a bounded recovery lifecycle.
Set<String> retain = new HashSet<>(undelivered.keySet());
collectConversationIds(queue, retain);
collectConversationIds(inFlightBatch, retain);
src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java:447
- This capacity check is not atomic with the later
computeIfAbsent. Submitters share the read lock, so several unseen conversations can all observe size 49,999 and then each insert a counter, exceeding the advertised hard cap by the number of concurrent callers. Reserve capacity atomically or serialize only the unseen-conversation insertion path.
if (!conversationSequences.containsKey(conversationId) && conversationSequences.size() >= MAX_TRACKED_CONVERSATIONS) {
LOGGER.warnv("Audit sequence table is full ({0} conversations, all with entries in flight) — "
+ "new conversations are recorded unsequenced until it drains", MAX_TRACKED_CONVERSATIONS);
return AuditEntry.UNSEQUENCED;
install.sh:646
- The failure mode described above can leave
$mf_targetas a directory from a previous Docker bind mount.rm -fcannot remove a directory and, underset -e, exits before the intendedfailmessage, so rerunning the installer does not clean up the stale mount path.
rm -f "$mf_target"
fail "Failed to download ${mf} (required for --with-monitoring).\n URL: ${mf_url}"
docs/changelog.md:88
- This changelog entry says the 575-name/273-file refactor and
ImportStyleTestshipped on this branch, but the PR description explicitly moved that work to #676 and this checkout contains noImportStyleTest. Since the changelog is the repository’s source of truth, move these claims to #676’s same-branch entry or rewrite them as follow-up work rather than recording unshipped changes here.
Four findings it raised in the review body rather than inline. Two were real bugs, one is a genuine constraint worth documenting, and one I disagree with on the evidence — recorded here rather than silently ignored. install.sh: `rm -f` cannot remove a DIRECTORY, and what is in the way is most likely exactly that — the stale mount path a previous failed run left behind. Under `set -euo pipefail` the failed rm becomes the script's exit point, so the user sees "rm: cannot remove ...: Is a directory" instead of the fail message explaining what to do, and the stale path survives to break the next run too. Reproduced before fixing; `rm -rf` restores the intended message. install.ps1 had the same shape: `Remove-Item -Force` does not remove a directory without -Recurse. Changelog placement violated AGENTS.md rule 8 — it must land on the branch carrying the work it documents. After the #675 split, the entries here still claimed the 575-name refactor and ImportStyleTest, which ship on chore/inline-fqn-cleanup. Those claims are removed; the branch no longer records work it did not carry. The sequence-table cap is a threshold, not a hard ceiling: the check in nextSequence is deliberately not atomic with the insert that follows, so concurrent submitters can overshoot slightly. Making it exact means serialising the insert path on every new conversation to enforce a bound that is a memory heuristic, not a correctness property. Said so where it is checked and where it is declared, instead of implying a guarantee that does not hold. On the fourth — that pinned undelivered conversations could leave every new conversation UNSEQUENCED until restart — the retention is real but that consequence is not reachable, and the suggested remedy is unsound. Re-seeding a conversation with a dead-lettered gap cannot work: countByConversation counts persisted rows, so with sequences 0-9 where 3 and 5 never landed the count is 8 while the next free position is 10, and it would hand out 8 and 9 again. Accounting for the highest undelivered position does not help either (max(8, 6) is still 8); a sound re-seed needs a maxSequence() the store interface does not expose. The pin is therefore correct, and bounded: undeliveredTracked counts SEQUENCES, so at most MAX_TRACKED_UNDELIVERED (10,000) conversations can be pinned against a 50,000 table, leaving 80% evictable. undeliveredPinCannotExhaustTheTable pins that headroom so raising one cap past the other fails the build rather than silently stranding conversations.
|
Picked up the four suppressed comments from the latest Copilot review — thanks, two were real bugs. Addressed in 3267b75.
Changelog placement (real, and a rule violation). Correct — AGENTS.md rule 8 requires the entry on the branch carrying the work. Splitting #675 left the 575-name/ The non-atomic capacity check. Confirmed, and deliberate — now said out loud rather than implied. Concurrent submitters share the read lock, so N can each observe one slot left and each insert. The overshoot is bounded by concurrency, self-corrects at the next eviction, and costs one The undelivered pin — retention is real, the consequence isn't, and the suggested fix is unsound. Worth spelling out:
So the pin stays, but the reasoning is now in the code instead of my head, and Suite re-run at baseline; |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
… arithmetic github-code-quality flagged undeliveredPinCannotExhaustTheTable as a useless comparison, and it was right about the shape: both operands are compile-time constants, so `10_000 < 50_000` folds to `true` and the assertion executes nothing. It would still have failed had someone edited a constant — the test recompiles against the new value — but that is a weak guarantee to rest an invariant on, and it cannot be mutation-checked at all. Replaced with the behavioural test it suggested. A store outage dead-letters a conversation so position 0 is consumed but never persisted; the store then recovers and the table is filled and flushed. The test asserts the persisted conversations are reclaimed while the pinned one is retained, that a later conversation still receives a real chain position rather than UNSEQUENCED, and that the pinned chain resumes at 1 rather than reusing the dead-lettered 0. That last assertion is the one the arithmetic could never make. Mutation-checked by dropping `undelivered.keySet()` from the retain set: the counter survived, so the chain resumes past the dead-lettered 0 rather than reusing it ==> expected: <1> but was: <0> which is the duplicate this whole mechanism exists to prevent.
|
Addressed in 580d442 — the flag was correct about the shape, and the suggested replacement is genuinely stronger. Both operands were compile-time constants, so It is now the behavioural test you described: a store outage dead-letters a conversation so position 0 is consumed but never persisted, the store recovers, the table fills and flushes. It then asserts the persisted conversations are reclaimed while the pinned one is retained, that a later conversation still receives a real chain position rather than That last assertion is the one arithmetic could never make. Mutation-checked by dropping which is exactly the duplicate this mechanism exists to prevent. |
|
For the record: the Verified against current No action needed on this one. |
# Conflicts: # docs/changelog.md
A full critical review of the repository, and the fixes for what it found. Split out of #675 so
automated review can actually run — that PR carried a 263-file mechanical refactor alongside these
changes, and both CodeRabbit (>100 files) and Copilot (>300) declined to review it. The refactor is
stacked in #676.
Most of what the review looked for was not there — no swallowed exceptions, no non-thread-safe
statics, no mutable state in the singleton lifecycle tasks, zero
@Disabledtests across 14,000+.The findings are few, but one matters, and the pattern in the rest is worth naming: nearly every
problem sat in something no automated check covered.
The one that matters
The audit ledger could report itself as tampered under load.
AuditLedgerServicecaps its sequence table at 50,000 conversations and used toclear()the wholething on overflow, on the stated reasoning that re-seeding from
countByConversationwas "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 retried — so a conversation with queued entries has consumed chain
positions the store cannot see yet. Re-seeding from the store count hands the same position out
twice, and the verifier grades a duplicate exactly like a gap:
The
undeliveredtable exists so the ledger's own back-pressure cannot read as tampering, but itonly exculpates gaps; duplicates had no such channel. On a busy deployment the queue is never
empty, so essentially every overflow produced them →
ChainStatus.BROKEN: the ledger accusing thedeployment of deleting audit records because its own bookkeeping wrapped around.
Eviction now drops only counters whose positions are all accounted for — persisted, or attributed in
undelivered. AReentrantReadWriteLockspans "position consumed" → "entry visible in the queue"against eviction (that window contains HMAC and Ed25519 signing). Still-full-after-eviction yields
UNSEQUENCED, degrading the window toUNAVAILABLE("cannot be established") rather than to anaccusation.
Writing the tests found a second defect in that fix:
flush()published its in-flight batchafter draining the queue, leaving a window where entries were in neither collection. Drain and
publish now happen under the same lock.
Also fixed
HUMAN_DECIDESblamed a feature that ships. The rejection read "needs human group members(I6), which are not available yet" — ~150 lines below the same file's I6 matrix that validates
those members. They shipped in 10c; the real blocker is the missing resume path for a paused
tie-break.
SafeHttpClientdocumented a guarantee it did not provide. It claimed an overall wall-clocktimeout across all hops, but the budget is only checked between hops, so a hop that trickles its
body hung forever. Both initial and redirect hops are now bounded.
elapsedNanos * limitoverflowed in longarithmetic after ~107 idle days, driving tokens negative so
tryAcquirerefused every later call.ConversationStepRunnerregistered the in-flight conversation outside thetrythat unregisters it.The gaps outside the build
install.sh/install.ps1had no verification of any kind — 95 KB of shipped script, theREADME's headline
curl | bashpath, in no path filter (so an installer-only PR skipped thewhole pipeline) with no lint, test, or syntax parse anywhere. New
shell-lintjob;install.shturned out already clean at ShellCheck's warning severity, so the gate lands green.
CI/CD, so nothing failed and it would have approved with a body claiming all checks passed on a
commit that was never built. Required jobs are now checked by name. Docs-only PRs still approve —
a job skipped by its
ifdoes report a check-run.planning/*.mdcomputed../and../../as if it lived under
docs/, so../../AGENTS.mdpointed outside the repository. The rest brokethe onboarding tutorials, which linked into a
.gitbook/assets/directory that does not exist;those now import EDDI's own
/openapiinto Postman, which cannot rot the way a committedcollection did.
CannotExecuteException(no references anywhere) andILogoutEndpoint(a JAX-RSinterface with no implementation, advertising
/user/isAuthenticatedto OpenAPI and servingnothing).
grafana.dbwas committed, in agrafana-data/directory orphaned by the move to anamed Docker volume. Its
eddi-operations.jsonturned out to be a richer dashboard than theprovisioned one (21 panels, "Operations Command Center") being maintained while visible to nobody
— so it was rescued into
docs/monitoring/and wired into the compose mount and both installers,rather than deleted with the rest.
Verification
Every fix has a regression test that was mutation-checked — the fix reverted, the test confirmed
to fail with the message it would show whoever broke it:
expected <[0, 1]> but was <[0, 0]>— the duplicate itselfexpected <1> but was <-1>(UNSEQUENCED)HUMAN_DECIDESmessageMcp*Toolsclass missing fromTOOL_CLASSESDocumentationLinksTestguards a class of bug rather than one defect — link rot is invisible toevery other check here, which is how 38 accumulated. It earned its place immediately: CI caught a
link the local run could not, because
../security.mdresolves to the rootSECURITY.mdoncase-insensitive Windows. The link was wrong (its own text said
docs/security.md); the test nowcompares on-disk casing too, so every platform gives CI's verdict.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation