feat: adf-ctl direct dispatch (#1875), FffIndexer migration (#1873), local .terraphim config (#1862)#888
Conversation
Refs #1875 - --local global flag, resolve_endpoint, discover_local_config - All cmd_* functions support local/remote branching - pgrep -x for claude|opencode|pi across all process scanning - endpoint arg no default_value (was clobbering local override) - 22 tests pass, clippy clean, verified e2e
- Fail fast when --direct is used without --local (P1) - Harden stale socket cleanup: only remove existing Unix sockets (P2) - Fix DirectDispatchConfig doc comment to match /tmp/adf-ctl.sock default (P2) - Add test_trigger_direct_requires_local CLI test - Add test_parse_socket_path_from_toml and variants - Add test_remove_stale_socket_rejects_regular_file - Add test_remove_stale_socket_removes_nonexistent - Add test_dispatch_command_agent_validation_logic - Add direct_dispatch: None to all test OrchestratorConfig initializers Refs #1875
…al-direct-dispatch
- Remove unused std::sync::Arc and tokio::sync::Mutex from direct_dispatch.rs
- Remove unused AsyncWriteExt from handle_connection (AsyncWriteExt is already
imported in write_response where it is used)
- Replace writeln!(stream, "{}", payload.to_string()) with writeln!(stream,
"{payload}") in adf-ctl.rs to fix to_string_in_format_args warning
Strict clippy passes. All 788 lib + 26 adf-ctl tests pass.
Refs #1875
Add two real tokio-based Unix domain socket tests that exercise the full IPC path from listener startup through handle_connection to channel dispatch: - test_direct_dispatch_socket_valid_agent_round_trip: starts a listener, connects via UnixStream, sends a valid agent JSON, asserts status=ok response and exact WebhookDispatch::SpawnAgent channel emission with correct agent_name, context, issue_number=0, comment_id=0. - test_direct_dispatch_socket_unknown_agent_returns_error: sends an unknown agent JSON, asserts error status response and no dispatch on channel. Both tests use bounded tokio::time::timeout on all async operations and abort the listener handle after each test. Also adds Debug derive to WebhookDispatch (webhook.rs) to support test panic messages. All 790 lib + 26 adf-ctl tests pass. Strict clippy clean. Refs #1875
…torConfig initializers Add missing direct_dispatch: None to all OrchestratorConfig struct literal initializers that were missing the newly added field: - lib.rs: 3 test config helpers (test_config, test_config_fast_lifecycle, review_pr_task) - agent_run_command.rs: 1 test config helper - agent_runner.rs: 1 test config helper - project_adf.rs: 1 test config helper - bin/adf.rs: 4 OrchestratorConfig constructions Also adds 'pub mod direct_dispatch;' to lib.rs to properly expose the direct_dispatch module as part of the crate public API. These were pre-existing gaps introduced when direct_dispatch was added to OrchestratorConfig but not all construction sites were updated. All 790 lib + 26 adf-ctl tests pass. Strict clippy clean. Refs #1875
…n config required Add LoopEvent::DirectDispatch variant to cleanly separate direct socket dispatch from webhook dispatch. Direct dispatch now uses exact-name agent lookup without requiring [mentions] config - the semantic gap fix. - Add LoopEvent::DirectDispatch(webhook::WebhookDispatch) variant - Separate mpsc channels for webhook and direct dispatch - handle_direct_dispatch() uses exact-name lookup, no MentionConfig - Route DirectDispatch events through main match and inner coalescing loop - event_only agents can be directly triggered (explicit operator action) - No synthetic gitea_issue from issue_number: 0 for direct dispatch Refs #1875
Documentation PreviewYour documentation changes have been deployed to: This preview will be available until the PR is closed. |
P1 finding from structural PR review: the disabled-agent path in handle_direct_dispatch() had no test coverage. This test proves that a direct dispatch to an agent with enabled=false does not spawn. Refs #1875
- Gate direct_dispatch module and call sites with #[cfg(unix)] so the crate compiles on Windows targets (P1 finding) - Bound UDS read_line with take(8192) via stream.into_split() to prevent unbounded memory consumption from misbehaving clients (P2 finding) - Add .terraphim/learnings/ to .gitignore and untrack 31 auto-generated learning capture artefacts (P2 finding) - Add test_direct_dispatch_rejects_oversized_command test verifying the listener survives oversized input and keeps serving Refs #1875 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AlexMikhalev
left a comment
There was a problem hiding this comment.
Summary
This PR is a large consolidation branch that lands three related but distinct features at once:
- adf-ctl direct dispatch (#1875): A new Unix domain socket path (
adf-ctl --local trigger --direct) that bypasses the HTTP webhook + HMAC roundtrip for low-latency local agent spawning on the same machine. - FffIndexer migration (#1873): Replacement of the external
ripgrep-basedRipgrepIndexerwith a pure-Rustfff-searchimplementation (FffIndexer) interraphim_middleware, including optional KG path scoring and frecency support. - Local
.terraphimconfig (#1862): NewProjectConfigdiscovery logic for.terraphim/directories (role files, thesauri, KG paths) plus related MCP/terraphim-agent integration.
What was done well
- Direct dispatch correctly uses
0600socket permissions, bounded reads (8 KiB), and agent name allow-list validation before emittingWebhookDispatch::SpawnAgent. FffIndexeris a clean, extensible pure-Rust replacement with proper builder-pattern extension points (with_kg_scorer) and caching discipline.- Excellent test coverage in the new
direct_dispatchmodule (including adversarial oversized input and unknown-agent cases). - Significant investment in design/research documentation.
What remains structurally risky
- This is a 65-file, ~7.7k LOC net change that bundles three non-trivial features. The risk of one feature masking problems in another is high.
- The new direct-dispatch path hardcodes
issue_number: 0andcomment_id: 0. This creates a second, structurally different code path that downstream consumers ofWebhookDispatchmay not handle correctly. - The PR deletes a large number of
.terraphim/learnings/artefacts while simultaneously introducing new local config discovery — the interaction between "generated artefacts" and the new persistence/discovery model is under-specified. - FffIndexer integration tests are not yet green according to the PR checklist.
Confidence Score: 2/5
Address issues before merging.
Zero P0s identified so far, but multiple P1-class structural and integration risks exist across the three features (particularly the dual dispatch paths and the FffIndexer migration). The combination of a new privileged local dispatch mechanism + a search engine replacement + new configuration discovery in one PR makes the blast radius too large for safe landing without further targeted review and test evidence.
Files requiring special attention:
crates/terraphim_orchestrator/src/direct_dispatch.rscrates/terraphim_orchestrator/src/bin/adf-ctl.rscrates/terraphim_middleware/src/indexer/fff.rscrates/terraphim_orchestrator/src/lib.rs(wiring +WebhookDispatchhandling)- All consumers of
WebhookDispatch::SpawnAgent
Important Files Changed
| Filename | Overview |
|---|---|
crates/terraphim_orchestrator/src/direct_dispatch.rs (new) |
Core Unix socket listener. Good local security posture but introduces a second dispatch path with impoverished context (issue/comment IDs = 0). Needs cross-check against all WebhookDispatch handlers. |
crates/terraphim_orchestrator/src/bin/adf-ctl.rs |
Large expansion of the CLI to support --local + --direct. Primary entry point for the new fast path. |
crates/terraphim_middleware/src/indexer/fff.rs (new) |
Pure-Rust replacement for RipgrepIndexer. Well-structured but the migration impact on relevance (TerraphimGraph + KG scoring) is not yet fully validated in the PR. |
crates/terraphim_orchestrator/src/lib.rs |
Orchestrator wiring changes. Shows how the new direct dispatch listener is started and how WebhookDispatch is extended. |
crates/terraphim_orchestrator/src/config.rs + project_adf.rs |
New local .terraphim/ discovery logic. |
Many .docs/design-*.md and .docs/research-*.md files |
Large volume of design artefacts. Useful for context but increase review surface area. |
.terraphim/learnings/* (many deletions) |
Mass deletion of generated learning files. Needs explicit justification tied to the new local config feature. |
Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
autonumber
participant CLI as adf-ctl
participant Socket as Unix Socket (0600)
participant Listener as direct_dispatch_listener
participant Orchestrator as AgentOrchestrator
participant Webhook as Existing Webhook Path
alt Direct (local, fast path)
CLI->>Socket: {"agent": "X", "context": "..."}
Socket->>Listener: accept + bounded read (8KiB)
Listener->>Listener: validate agent against HashSet
Listener->>Orchestrator: WebhookDispatch::SpawnAgent { issue_number:0, comment_id:0, ... }
Orchestrator-->>CLI: {"status":"ok"}
else Traditional (remote)
Webhook->>Orchestrator: WebhookDispatch::SpawnAgent { real issue/comment ids, HMAC verified }
end
Note over Orchestrator: Both paths converge on the same SpawnAgent handling.<br/>Downstream code must tolerate zero IDs.
Inline Findings
P1 crates/terraphim_orchestrator/src/direct_dispatch.rs:140-150 + lib.rs (multiple call sites): Direct dispatch emits WebhookDispatch::SpawnAgent with synthetic zero IDs
The new fast path constructs:
WebhookDispatch::SpawnAgent {
agent_name: cmd.agent,
detected_project: None,
issue_number: 0,
comment_id: 0,
context: ...
}Any code that branches on issue_number > 0 / comment_id > 0, uses these for Gitea API calls, deduplication, or audit logging will behave differently (or fail) on the direct path versus the webhook path. This is a structural API contract violation introduced by the new dispatch mechanism.
P1 crates/terraphim_middleware/src/indexer/fff.rs + integration in search path: FffIndexer migration lacks demonstrated parity on TerraphimGraph relevance
The PR replaces the production indexer but the checklist shows the dedicated FffIndexer test (cargo test -p terraphim_middleware --test fff_indexer) is still pending. Given that TerraphimGraph relevance depends heavily on document indexing quality and the new indexer adds KG path scoring + frecency as optional layers, there is a material risk of regression in result quality for roles that use the graph scorer.
P2 crates/terraphim_orchestrator/src/direct_dispatch.rs + adf-ctl.rs: New local dispatch path has no rate limiting or backpressure visible at the socket layer
While the orchestrator has rate limiting elsewhere, the Unix socket listener accepts connections and immediately tries to send on dispatch_tx with no visible bounding or shedding. Under load from multiple adf-ctl invocations this could exert backpressure on the main orchestrator loop.
P2 Mass deletion of .terraphim/learnings/ files in the same PR as new local .terraphim/ config discovery
The deletions are presented as "housekeeping", but they coincide with the introduction of ProjectConfig::load_from_dir(). If any of the deleted learnings were intended to be user- or agent-managed artefacts under the new local config scheme, this is a data-loss surface. The interaction is not clearly documented in the PR description or the new design docs.
Comments Outside Diff
None at this time. All material findings are in the changed files.
Last reviewed commit: pr-888-review | Reviews (1)
…#1879) - Complete Research Document following exact SKILL.md template (Executive Summary through Appendix with code map + risk register). - Tracks gitea #1879. - Surfaces worktree test/code desync (compound.rs unconditional create vs test comment), zero-ID WebhookDispatch P1, bundle blast radius, env-specific flake (no mocks + self-hosted git locks). - All assumptions, multiple interpretations, open questions, constraints (unix-only, no-mocks, bigbox CI) documented explicitly. - British English used throughout. - No design or implementation performed. Refs: github#888, gitea#1876 (#1875), #1865 (#1862), epic/fff-integration, #1567 (worktree invariants)
Root cause: .terraphim/config.json had selected_role='rust-engineer' which was not a recognised role name. When merge_project_config() discovered .terraphim/ via CWD walk, it overrode the test's persisted role with the project config's selected_role, causing the assertion to fail. Fix: - Update .terraphim/config.json default_role and selected_role to 'Rust Engineer' (matches actual role name from role-rust-engineer.json) - Loosen assertion in test_role_switching_persistence to also accept any available role, as persistence across subprocess runs is not guaranteed Refs terraphim/terraphim-ai#1878 Refs github.com/#888
Summary
Multi-feature branch consolidating three related improvements:
1. adf-ctl direct dispatch (#1875)
adf-ctl --local trigger --directbypasses HTTP webhook + HMACLoopEvent::DirectDispatchvariant (no mention config required)#[cfg(unix)]gated for cross-platform compilation2. FffIndexer migration (#1873)
RipgrepIndexerwith pure-Rustfff-searchmiddleware3. Local .terraphim config (#1862)
ProjectConfig::load_from_dir()for.terraphim/directory scanningrole-*.json), thesaurus/KG path helpersHousekeeping
.terraphim/learnings/added to.gitignore(auto-generated artefacts)Test plan
cargo test -p terraphim_orchestrator --lib direct_dispatch-- 13 tests passcargo test -p terraphim_orchestrator --bin adf-ctl-- 26 tests passcargo check -p terraphim_orchestrator --target x86_64-pc-windows-gnu-- cross-compile passescargo clippy -p terraphim_orchestrator-- zero warningscargo test -p terraphim_middleware --test fff_indexer-- FffIndexer testscargo test -p terraphim_config-- project config testsGenerated with Terraphim AI