feat: add iii-introspect worker - #11
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request introduces the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
System topology and workflow introspection worker. 9 functions: - introspect::functions/workers/triggers — list registered entities - introspect::topology — full system map (cached) - introspect::diagram — mermaid flowchart (filters engine internals, summary for 30+ nodes) - introspect::health — orphaned functions, empty workers, duplicate IDs - introspect::trace_workflow — trace a specific function's dependency chain - introspect::explain — natural language business explanation of a function/worker Uses iii primitives: State (topology cache), Cron (refresh), HTTP (9 endpoints). 20 tests, 0 warnings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
introspect/src/config.rs (1)
6-7: Consider validating cron expression at config load time.The
cron_topology_refreshis stored as a plainStringwith no validation. An invalid cron expression will only be detected when the trigger is registered with the engine, which may be harder to debug. Consider parsing and validating the cron expression during config loading.🛡️ Example validation approach
// Add to dependencies: cron = "0.12" use cron::Schedule; use std::str::FromStr; pub fn load_config(path: &str) -> Result<IntrospectConfig> { let contents = std::fs::read_to_string(path)?; let config: IntrospectConfig = serde_yaml::from_str(&contents)?; // Validate cron expression Schedule::from_str(&config.cron_topology_refresh) .map_err(|e| anyhow::anyhow!("invalid cron expression: {}", e))?; Ok(config) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@introspect/src/config.rs` around lines 6 - 7, Validate the cron_topology_refresh string when loading IntrospectConfig rather than later: add the cron crate dependency, and in the config loader function (e.g., load_config or wherever IntrospectConfig is deserialized) call cron::Schedule::from_str(&config.cron_topology_refresh) and return a clear error (wrap with anyhow or your project's error type) if parsing fails; keep the field signature pub cron_topology_refresh: String and reference default_cron for defaults but ensure invalid cron expressions are rejected at config load time.introspect/config.yaml (1)
2-2: TTL mismatch between config file and code defaults.
config.yamlsetscache_ttl_seconds: 300(5 minutes), but the default inintrospect/src/config.rsandintrospect/src/manifest.rsis30seconds. This 10x difference could cause confusion during deployment. Consider aligning these values or documenting the intentional difference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@introspect/config.yaml` at line 2, The TTL in config.yaml (cache_ttl_seconds: 300) conflicts with the 30s defaults used in introspect/src/config.rs and introspect/src/manifest.rs; pick one source of truth and align them—either update config.yaml to 30 or change the default constants (e.g., the cache TTL constant or default in functions within config.rs and manifest.rs) to 300, and if the disparity is intentional, add a short comment in config.yaml or in the default constant declarations explaining why runtime defaults differ from the file value.introspect/README.md (1)
40-46: Add language specifier to fenced code block.The code block showing CLI options is missing a language specifier, which aids syntax highlighting and accessibility tools.
📝 Suggested fix
-``` +```text Options: --config <PATH> Path to config.yaml [default: ./config.yaml] --url <URL> WebSocket URL of the iii engine [default: ws://127.0.0.1:49134] --manifest Output module manifest as JSON and exit -h, --help Print help</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@introspect/README.mdaround lines 40 - 46, The fenced code block that shows
the CLI options is missing a language specifier; update the triple-backtick
fence around the Options block in README.md to include a language tag (e.g.,
"text") so the block becomestext ...to enable proper syntax
highlighting and accessibility tools for the CLI options listing.</details> </blockquote></details> <details> <summary>introspect/src/functions/state.rs (1)</summary><blockquote> `19-33`: **Consider adding explicit timeouts for resilience.** Both `state_get` and `state_set` use `timeout_ms: None`. If the engine's state operations hang or become slow, these calls could block indefinitely. Consider adding reasonable timeouts for better operational resilience. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/state.rs` around lines 19 - 33, Add a bounded timeout to the TriggerRequest calls to avoid indefinite hangs: update state_set (and similarly state_get) to set timeout_ms to a sensible value (e.g. Some(5_000) or use a shared constant like STATE_TIMEOUT_MS) instead of None so the trigger call fails fast on slow/hung engine; ensure you reference the TriggerRequest construction in the state_set and state_get functions and use an appropriate u64 millisecond value or a configurable constant. ``` </details> </blockquote></details> <details> <summary>introspect/src/functions/topology.rs (1)</summary><blockquote> `86-102`: **Optional: Consider deterministic ordering for `functions_per_worker`.** `HashMap` iteration order is non-deterministic, so the `functions_per_worker` array order varies between invocations. This has no functional impact but could cause unnecessary cache invalidations if comparing outputs or confuse debugging. Using `BTreeMap` or sorting entries before serialization would provide consistent output. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/topology.rs` around lines 86 - 102, functions_per_worker is currently a HashMap so its iteration order (used to build fpw_entries) is non-deterministic; change to a deterministic collection or sort before serializing: either replace HashMap<String, usize> functions_per_worker with BTreeMap<String, usize> or, after building functions_per_worker from named_workers, produce a sorted Vec by key (worker name) and then map to fpw_entries; update references to functions_per_worker, named_workers, and fpw_entries accordingly so the output order is stable. ``` </details> </blockquote></details> <details> <summary>introspect/SPEC.md (1)</summary><blockquote> `222-230`: **Add language specifier to fenced code block.** The CLI usage code block is missing a language identifier. This triggers the MD040 linting warning. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text iii-introspect [OPTIONS] Options: --config <PATH> Path to config.yaml [default: ./config.yaml] --url <URL> WebSocket URL of the III engine [default: ws://127.0.0.1:49134] --manifest Output module manifest as JSON and exit -h, --help Print help ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@introspect/SPEC.mdaround lines 222 - 230, Update the fenced code block that
shows the CLI usage for "iii-introspect [OPTIONS]" by adding a language
specifier to the opening fence (e.g., change the openingtotext) so the
block is explicitly marked as plain text; edit the block that contains the lines
starting with "iii-introspect [OPTIONS]" and the Options list to include the
language tag.</details> </blockquote></details> <details> <summary>introspect/src/functions/diagram.rs (1)</summary><blockquote> `39-44`: **Consider sanitizing additional Mermaid-special characters.** The current `sanitize_id` handles `::`, `-`, `.`, and space, but Mermaid syntax can be broken by characters like `[`, `]`, `{`, `}`, `|`, `>`, `<`, `"`, and `\n` in node IDs. If function or worker IDs could contain these characters, the generated diagram may have syntax errors. <details> <summary>🛡️ More robust sanitization</summary> ```diff fn sanitize_id(id: &str) -> String { id.replace("::", "_") .replace('-', "_") .replace('.', "_") .replace(' ', "_") + .replace('[', "_") + .replace(']', "_") + .replace('{', "_") + .replace('}', "_") + .replace('|', "_") + .replace('>', "_") + .replace('<', "_") + .replace('"', "_") + .replace('\n', "_") } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/diagram.rs` around lines 39 - 44, The sanitize_id function currently only replaces "::", '-', '.', and spaces which can still allow Mermaid-breaking characters; update sanitize_id to also replace or normalize characters such as '[', ']', '{', '}', '|', '>', '<', '"', '\\n' (newline), '\\', and any other non-alphanumeric characters into safe characters (e.g., '_') to ensure valid Mermaid node IDs. You can implement this by expanding the current chained .replace calls or, better, by using a single regex that maps any character not in [A-Za-z0-9_] to '_' inside sanitize_id to guarantee robustness for all function/worker IDs. ``` </details> </blockquote></details> <details> <summary>introspect/src/functions/trace.rs (4)</summary><blockquote> `161-165`: **Use `root_function_id` directly instead of extracting from chain.** `root_function_id` is already available and guaranteed to be the starting function. Extracting it from `chain.first()` is indirect and could return `""` if the chain were somehow empty (though unlikely given current logic). <details> <summary>♻️ Proposed fix</summary> ```diff Ok(serde_json::json!({ - "function_id": chain.first().and_then(|c| c.get("function_id")).and_then(|v| v.as_str()).unwrap_or(""), + "function_id": root_function_id, "chain": chain, "diagram": diagram, })) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/trace.rs` around lines 161 - 165, Replace the indirect extraction of the starting function ID from chain.first() with the already-available root_function_id: in the JSON construction use root_function_id instead of chain.first().and_then(...).unwrap_or(""), so the "function_id" field is populated directly from root_function_id (refer to variables root_function_id and chain in the function that returns Ok(serde_json::json!(...)) in trace.rs). ``` </details> --- `91-101`: **The variable `queue` uses LIFO (stack) behavior, not FIFO.** `Vec::pop()` removes from the end (LIFO/stack), but the variable is named `queue` which conventionally implies FIFO. This affects traversal order—you get depth-first rather than breadth-first exploration. If breadth-first (level-by-level) ordering is intended for the workflow trace, use `VecDeque` with `pop_front()`. If depth-first is acceptable, consider renaming to `stack` for clarity. <details> <summary>♻️ Option A: Use VecDeque for true FIFO queue behavior</summary> ```diff +use std::collections::VecDeque; let mut chain: Vec<Value> = Vec::new(); let mut visited: std::collections::HashSet<String> = std::collections::HashSet::new(); - let mut queue: Vec<String> = vec![root_function_id.clone()]; + let mut queue: VecDeque<String> = VecDeque::from([root_function_id.clone()]); let mut step = 0u32; - while let Some(current_fid) = queue.pop() { + while let Some(current_fid) = queue.pop_front() { ``` </details> <details> <summary>♻️ Option B: Rename to `stack` if DFS is intentional</summary> ```diff - let mut queue: Vec<String> = vec![root_function_id.clone()]; + let mut stack: Vec<String> = vec![root_function_id.clone()]; - while let Some(current_fid) = queue.pop() { + while let Some(current_fid) = stack.pop() { // ... - queue.push(other_t.function_id.clone()); + stack.push(other_t.function_id.clone()); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/trace.rs` around lines 91 - 101, The loop currently treats `queue: Vec<String>` as FIFO but uses `Vec::pop()` (LIFO), producing DFS; to get true FIFO BFS change the container to `std::collections::VecDeque<String>` (e.g., `let mut queue: VecDeque<String> = VecDeque::from([root_function_id.clone()])`) and replace `queue.pop()` with `queue.pop_front()` and uses of `push` with `push_back`; alternatively, if depth-first was intended, simply rename `queue` to `stack` (or `stack: Vec<String>`) to reflect LIFO semantics and keep using `pop()` — update references in the while loop and the `visited`/`step` logic accordingly. ``` </details> --- `139-156`: **The topic-matching logic finds peer subscribers, not upstream/downstream dependencies.** This logic enqueues functions that also subscribe to the same topic as the current function. However, two functions subscribing to the same topic are peer consumers—not a dependency chain. A true workflow trace would need to identify producer→consumer relationships (which function publishes to a topic vs which subscribes). If publish metadata isn't available, consider: 1. Documenting this limitation in the API response or comments 2. Renaming the output from "chain" to something like "related_functions" or "topic_graph" 3. Adding a note in the response indicating these are topic-based relationships, not causal dependencies <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/trace.rs` around lines 139 - 156, The current loop over func_triggers enqueues functions that also subscribe to the same topic (peer consumers) rather than producer→consumer dependencies; update the logic in trace.rs (the block iterating func_triggers, using func_triggers, triggers, queue, current_fid) to detect producers first (e.g., look for triggers with trigger_type == "publish" or a publish-like key in trigger.config) and only enqueue their subscriber function_ids, otherwise if publish metadata is unavailable either (A) add a comment and a note in the API response indicating these are topic-based related functions (not causal dependencies) or (B) rename the output field from "chain" to "related_functions" or "topic_graph" to reflect the limitation. Ensure all changes reference func_triggers, triggers, queue, and current_fid so reviewers can find the updated logic. ``` </details> --- `199-206`: **Edge label duplicates the trigger node label.** The trigger type is used both as the node content (`{{"{}"}}`) and as the edge label (`|{}|`), creating redundancy in the diagram. Consider using the trigger ID or a more descriptive label for one of them. <details> <summary>♻️ Example: Use trigger_id for node, type for edge</summary> ```diff if let Some(triggers) = triggers_by_function.get(fid) { for t in triggers { let trigger_safe = sanitize_id(&t.id); let label = &t.trigger_type; diagram.push_str(&format!( - " {}{{\"{}\"}} -->|{}| {}\n", - trigger_safe, label, label, safe_fid + " {}{{\"{}\"}} -->|{}| {}\n", + trigger_safe, t.id, label, safe_fid )); } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/trace.rs` around lines 199 - 206, The diagram code in the loop over triggers uses t.trigger_type for both the node content and the edge label (variables trigger_safe, label, safe_fid in trace.rs), causing redundancy; update the edge label to use the trigger's ID (e.g., t.id or sanitize_id(&t.id)) or another descriptive field instead of label so the node shows the type and the edge shows the identifier (adjust the format! call accordingly to use the chosen field for the |...| edge label while keeping the node content as label). ``` </details> </blockquote></details> <details> <summary>introspect/src/functions/explain.rs (2)</summary><blockquote> `248-265`: **Consider deduplicating the "inbound" function IDs and clarifying the semantic meaning.** Two potential improvements: 1. **Duplicate entries**: If a function has multiple subscribe triggers on topics that overlap with `our_topics`, the same `function_id` could appear multiple times in `inbound`. Consider collecting into a `HashSet` or deduplicating. 2. **Naming clarity**: The variable `inbound` and the explanation text "Connected to:" suggest upstream producers, but this logic finds peer functions that also *subscribe* to the same topics—not functions that *publish* to those topics. Consider renaming to `related_subscribers` or `peer_functions` and adjusting the explanation text to "Also subscribed to same topics:" for accuracy. <details> <summary>♻️ Optional: Deduplicate and clarify naming</summary> ```diff - let inbound: Vec<String> = all_triggers + let related_subscribers: Vec<String> = all_triggers .iter() .filter(|t| t.trigger_type == "subscribe") .filter(|t| { let topic = t.config.get("topic").and_then(|v| v.as_str()); let our_topics: Vec<&str> = func_triggers .iter() .filter(|ft| ft.trigger_type == "subscribe") .filter_map(|ft| ft.config.get("topic").and_then(|v| v.as_str())) .collect(); if let Some(topic) = topic { our_topics.contains(&topic) && t.function_id != func.function_id } else { false } }) .map(|t| t.function_id.clone()) + .collect::<std::collections::HashSet<_>>() + .into_iter() .collect(); ``` And update the explanation text: ```diff - if !inbound.is_empty() { + if !related_subscribers.is_empty() { explanation_parts.push(format!( - "Connected to: {}.", - inbound.join(", ") + "Also subscribed to same topics: {}.", + related_subscribers.join(", ") )); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/explain.rs` around lines 248 - 265, The current computation of inbound can produce duplicate function IDs and is semantically misnamed: change the collection to deduplicate (e.g., collect into a HashSet or use .unique by function_id) so each peer function_id appears once, and rename the variable from inbound to something clearer like related_subscribers or peer_functions; update any explanation text that prints "Connected to:" to instead say "Also subscribed to same topics:" (references: all_triggers, func_triggers, inbound, func.function_id). ``` </details> --- `269-272`: **Minor: `.to_lowercase()` on description may produce awkward natural language.** If the description contains proper nouns, acronyms (e.g., "HTTP", "API", "AWS"), or is already sentence-case, converting to lowercase could produce awkward output like "eval::metrics calculate metrics for a tracked function." Consider preserving the original casing or only lowercasing the first character if it begins with uppercase. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/explain.rs` around lines 269 - 272, The current join uses description.to_lowercase() which forces all-caps words and proper nouns to be lowercased; update the construction used in explanation_parts.push so it preserves original casing except for ensuring the first character is sentence-appropriate (lowercase only the first character if it is uppercase) rather than calling description.to_lowercase(). Locate the call in explanation_parts.push(format!(..., func.function_id, description.to_lowercase() )) and replace the .to_lowercase() usage with logic that leaves description intact but transforms only the first grapheme/character to lowercase (or simply use description unchanged) to avoid mangling acronyms and proper nouns like "HTTP", "API", "AWS". ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@introspect/src/main.rs:
- Line 417: Log message in tracing::info! currently claims "11 triggers" but
only 9 triggers are registered; update the message inside the tracing::info!
call to reflect the correct count or, better, build the message from the actual
registered counts (e.g., use the variables/collections that track functions and
triggers instead of a hardcoded string). Locate the
tracing::info!("iii-introspect registered 9 functions and 11 triggers, waiting
for invocations") call in main.rs and either change "11 triggers" to "9
triggers" or interpolate the real counts (e.g., registered_functions.len() and
registered_triggers.len()) so the log stays accurate.
Nitpick comments:
In@introspect/config.yaml:
- Line 2: The TTL in config.yaml (cache_ttl_seconds: 300) conflicts with the 30s
defaults used in introspect/src/config.rs and introspect/src/manifest.rs; pick
one source of truth and align them—either update config.yaml to 30 or change the
default constants (e.g., the cache TTL constant or default in functions within
config.rs and manifest.rs) to 300, and if the disparity is intentional, add a
short comment in config.yaml or in the default constant declarations explaining
why runtime defaults differ from the file value.In
@introspect/README.md:
- Around line 40-46: The fenced code block that shows the CLI options is missing
a language specifier; update the triple-backtick fence around the Options block
in README.md to include a language tag (e.g., "text") so the block becomes
text ...to enable proper syntax highlighting and accessibility tools for
the CLI options listing.In
@introspect/SPEC.md:
- Around line 222-230: Update the fenced code block that shows the CLI usage for
"iii-introspect [OPTIONS]" by adding a language specifier to the opening fence
(e.g., change the openingtotext) so the block is explicitly marked as
plain text; edit the block that contains the lines starting with "iii-introspect
[OPTIONS]" and the Options list to include the language tag.In
@introspect/src/config.rs:
- Around line 6-7: Validate the cron_topology_refresh string when loading
IntrospectConfig rather than later: add the cron crate dependency, and in the
config loader function (e.g., load_config or wherever IntrospectConfig is
deserialized) call cron::Schedule::from_str(&config.cron_topology_refresh) and
return a clear error (wrap with anyhow or your project's error type) if parsing
fails; keep the field signature pub cron_topology_refresh: String and reference
default_cron for defaults but ensure invalid cron expressions are rejected at
config load time.In
@introspect/src/functions/diagram.rs:
- Around line 39-44: The sanitize_id function currently only replaces "::", '-',
'.', and spaces which can still allow Mermaid-breaking characters; update
sanitize_id to also replace or normalize characters such as '[', ']', '{', '}',
'|', '>', '<', '"', '\n' (newline), '\', and any other non-alphanumeric
characters into safe characters (e.g., '') to ensure valid Mermaid node IDs.
You can implement this by expanding the current chained .replace calls or,
better, by using a single regex that maps any character not in [A-Za-z0-9] to
'_' inside sanitize_id to guarantee robustness for all function/worker IDs.In
@introspect/src/functions/explain.rs:
- Around line 248-265: The current computation of inbound can produce duplicate
function IDs and is semantically misnamed: change the collection to deduplicate
(e.g., collect into a HashSet or use .unique by function_id) so each peer
function_id appears once, and rename the variable from inbound to something
clearer like related_subscribers or peer_functions; update any explanation text
that prints "Connected to:" to instead say "Also subscribed to same topics:"
(references: all_triggers, func_triggers, inbound, func.function_id).- Around line 269-272: The current join uses description.to_lowercase() which
forces all-caps words and proper nouns to be lowercased; update the construction
used in explanation_parts.push so it preserves original casing except for
ensuring the first character is sentence-appropriate (lowercase only the first
character if it is uppercase) rather than calling description.to_lowercase().
Locate the call in explanation_parts.push(format!(..., func.function_id,
description.to_lowercase() )) and replace the .to_lowercase() usage with logic
that leaves description intact but transforms only the first grapheme/character
to lowercase (or simply use description unchanged) to avoid mangling acronyms
and proper nouns like "HTTP", "API", "AWS".In
@introspect/src/functions/state.rs:
- Around line 19-33: Add a bounded timeout to the TriggerRequest calls to avoid
indefinite hangs: update state_set (and similarly state_get) to set timeout_ms
to a sensible value (e.g. Some(5_000) or use a shared constant like
STATE_TIMEOUT_MS) instead of None so the trigger call fails fast on slow/hung
engine; ensure you reference the TriggerRequest construction in the state_set
and state_get functions and use an appropriate u64 millisecond value or a
configurable constant.In
@introspect/src/functions/topology.rs:
- Around line 86-102: functions_per_worker is currently a HashMap so its
iteration order (used to build fpw_entries) is non-deterministic; change to a
deterministic collection or sort before serializing: either replace
HashMap<String, usize> functions_per_worker with BTreeMap<String, usize> or,
after building functions_per_worker from named_workers, produce a sorted Vec by
key (worker name) and then map to fpw_entries; update references to
functions_per_worker, named_workers, and fpw_entries accordingly so the output
order is stable.In
@introspect/src/functions/trace.rs:
- Around line 161-165: Replace the indirect extraction of the starting function
ID from chain.first() with the already-available root_function_id: in the JSON
construction use root_function_id instead of
chain.first().and_then(...).unwrap_or(""), so the "function_id" field is
populated directly from root_function_id (refer to variables root_function_id
and chain in the function that returns Ok(serde_json::json!(...)) in trace.rs).- Around line 91-101: The loop currently treats
queue: Vec<String>as FIFO but
usesVec::pop()(LIFO), producing DFS; to get true FIFO BFS change the
container tostd::collections::VecDeque<String>(e.g.,let mut queue: VecDeque<String> = VecDeque::from([root_function_id.clone()])) and replace
queue.pop()withqueue.pop_front()and uses ofpushwithpush_back;
alternatively, if depth-first was intended, simply renamequeuetostack(or
stack: Vec<String>) to reflect LIFO semantics and keep usingpop()— update
references in the while loop and thevisited/steplogic accordingly.- Around line 139-156: The current loop over func_triggers enqueues functions
that also subscribe to the same topic (peer consumers) rather than
producer→consumer dependencies; update the logic in trace.rs (the block
iterating func_triggers, using func_triggers, triggers, queue, current_fid) to
detect producers first (e.g., look for triggers with trigger_type == "publish"
or a publish-like key in trigger.config) and only enqueue their subscriber
function_ids, otherwise if publish metadata is unavailable either (A) add a
comment and a note in the API response indicating these are topic-based related
functions (not causal dependencies) or (B) rename the output field from "chain"
to "related_functions" or "topic_graph" to reflect the limitation. Ensure all
changes reference func_triggers, triggers, queue, and current_fid so reviewers
can find the updated logic.- Around line 199-206: The diagram code in the loop over triggers uses
t.trigger_type for both the node content and the edge label (variables
trigger_safe, label, safe_fid in trace.rs), causing redundancy; update the edge
label to use the trigger's ID (e.g., t.id or sanitize_id(&t.id)) or another
descriptive field instead of label so the node shows the type and the edge shows
the identifier (adjust the format! call accordingly to use the chosen field for
the |...| edge label while keeping the node content as label).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `260c2edb-17a4-49dc-a290-34f5b7e6857d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 606a4d47e282b6a6e45554e6b80ad13c58a208b2 and a44322a6750975d5388d227be5d08554b9f66b03. </details> <details> <summary>📒 Files selected for processing (18)</summary> * `introspect/Cargo.toml` * `introspect/README.md` * `introspect/SPEC.md` * `introspect/build.rs` * `introspect/config.yaml` * `introspect/src/config.rs` * `introspect/src/functions/diagram.rs` * `introspect/src/functions/explain.rs` * `introspect/src/functions/functions.rs` * `introspect/src/functions/health.rs` * `introspect/src/functions/mod.rs` * `introspect/src/functions/state.rs` * `introspect/src/functions/topology.rs` * `introspect/src/functions/trace.rs` * `introspect/src/functions/triggers.rs` * `introspect/src/functions/workers.rs` * `introspect/src/main.rs` * `introspect/src/manifest.rs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
rohitg00
force-pushed
the
feat/introspect
branch
from
April 7, 2026 18:36
a44322a to
f35fed6
Compare
…ypes (queue→durable::subscriber, enqueue→iii::durable::publish)
- Drop features = ["otel"] (OTel always-on in 0.11.0) - Add metadata: None to RegisterTriggerInput and TriggerInfo literals (new required field in 0.11.0) - Fix subscribe test to use canonical trigger type durable::subscriber - 20 tests pass
Contributor
Author
|
Heads up — main is being bumped to
Release notes: https://github.com/iii-hq/iii/releases/tag/iii/v0.11.3 |
Was '11 triggers' — actual count is 9 (8 HTTP + 1 cron).
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
introspect/src/functions/workers.rs (1)
20-48: LGTM.Filter predicate, subtraction for
anonymous_connections(guaranteed non-negative sinceentriesis a subset ofworkers), and JSON shape all look correct.Optional: iterating with
.iter()and then buildingjson!values will clone eachString/Vecfield. Iflist_workers()returns an ownedVec, you could consume it with.into_iter()and move fields into the JSON to avoid those clones — negligible unless worker counts grow large.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@introspect/src/functions/workers.rs` around lines 20 - 48, The current code clones worker fields by iterating with workers.iter(); to avoid clones, capture total = workers.len() first, then consume the owned Vec by changing workers.iter().filter(...).map(...) to workers.into_iter().filter(...).map(...), updating the closures to take owned w, and compute anonymous_count = total - entries.len(); adjust references to workers, entries, anonymous_count, and the handle/iii.list_workers() call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@introspect/README.md`:
- Around line 42-48: The fenced code block in README.md is missing a language
tag and triggers markdownlint MD040; update the block that shows the CLI Options
to use a language of "text" by changing the opening triple backticks to ```text
so the CLI help output is treated as plain text (e.g., the block starting with
"Options:" should start with ```text and end with ```). Ensure only the opening
fence is modified and the block content remains unchanged.
In `@introspect/src/functions/diagram.rs`:
- Around line 39-44: sanitize_id currently normalizes IDs by simple character
replacement which can collapse distinct IDs and leaves labels unescaped for
Mermaid; modify the sanitize_id function to produce collision-safe node IDs by
prefixing with the node type, appending a stable hash (e.g. SHA1 or blake2b) of
the original id, and only minimally normalizing for Mermaid-safe characters, and
separately escape/display labels (escape quotes, brackets, backticks, newlines
and backslashes) before inserting into Mermaid markup; apply the same change to
the other ID-normalization/label-rendering spots in this file referenced in the
review so every node uses type-prefixed hashed IDs and escaped labels.
In `@introspect/src/functions/explain.rs`:
- Around line 248-265: The current inbound computation treats other
durable::subscriber triggers on the same topic as upstream inbound links
(variable inbound using all_triggers and func_triggers), which labels peer
consumers as inbound; fix it by first collecting our_topics from func_triggers
(as done) and then change the filter so you include triggers whose topic matches
and have a different function_id but explicitly exclude cases where the
candidate is a subscriber on a topic we also subscribe to (i.e., skip when
t.trigger_type == "durable::subscriber" && our_topics.contains(topic)); in other
words, allow matching topics but do not count same-topic durable::subscriber
peers as inbound (adjust the filter in the closure that builds inbound
accordingly).
- Around line 32-36: The current validation only errors when both selectors are
missing but allows both function_id and worker_name to be supplied, which can
return an explanation with the wrong context; change the check to enforce
exclusivity (XOR) between function_id and worker_name so the code returns an
error if both are provided, and update the error message to indicate that
exactly one of function_id or worker_name must be supplied; locate the
validation around the function_id and worker_name variables in explain.rs and
replace the existing is_none() check with a guard that errors on either both
None or both Some.
- Around line 61-70: The current building of func_to_worker overwrites previous
entries when multiple workers advertise the same function; change func_to_worker
from HashMap<String, String> to HashMap<String, Vec<String>> (or
HashSet<String>) and, when iterating over workers (use the existing workers
variable and w.name/w.functions), push/insert the worker name into the
vector/set instead of replacing it; apply the same change to the second
occurrence around lines 83-88 so callers can detect/report multiple hosts for a
function ID and adjust any downstream logic that expects a single host to handle
multiple hosts (e.g., join or choose reporting the ambiguity).
- Around line 226-234: The current mapping that builds trigger_details from
func_triggers returns the raw TriggerInfo.config which may leak secrets; instead
implement a helper function (e.g., public_trigger_config) that inspects
TriggerInfo.trigger_type and returns a whitelisted JSON shape (for types like
"http", "cron", "durable::subscriber", "state", etc.) and then replace the
mapping used to build trigger_details so it calls public_trigger_config(&t)
rather than embedding t.config directly; update describe_trigger to use this
helper when constructing explain output.
In `@introspect/src/functions/topology.rs`:
- Around line 86-102: The current functions_per_worker HashMap uses the display
name (w.name.unwrap_or_else(|| w.id.clone())) as the key which can overwrite
entries when multiple workers share the same name; change the aggregation to use
the stable worker id as the HashMap key (use w.id.clone() for keys in
functions_per_worker) or, alternatively, avoid aggregating by key at all and
build fpw_entries directly from named_workers so each entry contains both "id":
w.id and "name": w.name (and "function_count": w.function_count) to preserve all
workers; update references to functions_per_worker, named_workers, fpw_entries,
w.name, w.id, and w.function_count accordingly.
In `@introspect/src/functions/trace.rs`:
- Around line 139-156: The loop in trace.rs treats other_t entries with
trigger_type "durable::subscriber" and the same topic as downstream dependencies
(using func_triggers, triggers, current_fid and pushing into queue), which
incorrectly marks sibling subscribers as consumers; change the logic so you do
not enqueue same-topic subscribers—either skip adding other_t.function_id to
queue when other_t.trigger_type == "durable::subscriber" and topic matches, or
instead collect them into a separate "related_subscribers" metadata list
returned by the traversal; ensure only explicit publish/output relationships are
enqueued as downstream dependencies while preserving the same-topic subscribers
in a non-traversal metadata structure.
- Around line 168-205: The Mermaid nodes/IDs can collide and labels can break
the diagram; update sanitize_id and build_trace_mermaid so IDs are namespaced
and labels are escaped: ensure sanitize_id normalizes more characters (keep as
helper) but when constructing node/edge IDs prefix them (e.g. "fn_"+safe_fid for
function nodes, "trigger_"+sanitize_id(&t.id) for triggers,
"worker_"+sanitize_id(worker) for workers) so IDs are unique across types, and
pass displayed text (fid, worker, t.trigger_type, etc.) through a new
mermaid_label helper that escapes quotes, braces, pipes and newlines before
embedding in the node/edge label (use the prefixed sanitized ID only for the
node identifier and the escaped mermaid_label for the visible text in format
strings like {}[\"{}\"] or {}{{\"{}\"}} -->|{}| {}).
---
Nitpick comments:
In `@introspect/src/functions/workers.rs`:
- Around line 20-48: The current code clones worker fields by iterating with
workers.iter(); to avoid clones, capture total = workers.len() first, then
consume the owned Vec by changing workers.iter().filter(...).map(...) to
workers.into_iter().filter(...).map(...), updating the closures to take owned w,
and compute anonymous_count = total - entries.len(); adjust references to
workers, entries, anonymous_count, and the handle/iii.list_workers() call
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b887e64e-d280-4549-81ea-0d01fc108e72
⛔ Files ignored due to path filters (1)
introspect/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
introspect/Cargo.tomlintrospect/README.mdintrospect/build.rsintrospect/config.yamlintrospect/src/config.rsintrospect/src/functions/diagram.rsintrospect/src/functions/explain.rsintrospect/src/functions/functions.rsintrospect/src/functions/health.rsintrospect/src/functions/mod.rsintrospect/src/functions/state.rsintrospect/src/functions/topology.rsintrospect/src/functions/trace.rsintrospect/src/functions/triggers.rsintrospect/src/functions/workers.rsintrospect/src/main.rsintrospect/src/manifest.rs
✅ Files skipped from review due to trivial changes (5)
- introspect/config.yaml
- introspect/src/functions/mod.rs
- introspect/build.rs
- introspect/Cargo.toml
- introspect/src/functions/health.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- introspect/src/functions/functions.rs
- introspect/src/main.rs
Comment on lines
+5
to
+17
| **Plug and play:** Build with `cargo build --release`, then run `./target/release/iii-introspect --url ws://your-engine:49134`. It registers 9 functions and starts caching topology every 5 minutes. Call `introspect::trace_workflow` with any function ID to trace its dependency chain, or `introspect::explain` to get a business-level explanation of what it does and how it's triggered. | ||
|
|
||
| ## Functions | ||
|
|
||
| | Function ID | Description | | ||
| |---|---| | ||
| | `introspect::functions` | List all registered functions in the engine | | ||
| | `introspect::workers` | List all connected workers | | ||
| | `introspect::triggers` | List all registered triggers | | ||
| | `introspect::topology` | Full system topology with stats (cached with TTL) | | ||
| | `introspect::diagram` | Generate a Mermaid flowchart of the system topology | | ||
| | `introspect::health` | Health check for orphaned functions, empty workers, duplicate IDs | | ||
| | `introspect::topology_refresh` | Cron-triggered cache refresh (internal, not exposed via HTTP) | |
There was a problem hiding this comment.
Document all public endpoints and correct the HTTP method wording.
The intro says 9 functions are registered, but the table omits introspect::trace_workflow and introspect::explain. Also, “all public functions exposed as GET endpoints” conflicts with tracing/explain-style handlers that accept payloads. Please list all 9 functions and describe the actual methods/payloads.
Also applies to: 21-23
…explain selector, secret-safe trigger config, topology worker-id key
diagram.rs:
- Replace sanitize_id with type-prefixed + hashed node ids
(fn_…/worker_…/trigger_…) so two raw ids that collapse under simple
char normalization (e.g. 'foo::bar' vs 'foo--bar') still produce
distinct Mermaid nodes.
- Add mermaid_label() that HTML-escapes double quotes, pipes, brackets,
braces, angle brackets, and collapses newlines so user-supplied names
can't break the Mermaid parser.
- Use the new helpers everywhere node ids or visible labels are emitted.
explain.rs:
- Selector is now exclusive-or between function_id and worker_name.
Supplying both used to silently pick one and ignore the other,
returning an explanation with the wrong context.
- func_to_worker: HashMap<String,String> -> HashMap<String,Vec<String>>.
Multiple workers that advertise the same function_id (replicas) no
longer collapse; when more than one host exists the worker field
reports the ambiguity.
- public_trigger_config(): whitelist http/cron/durable::subscriber/state
config fields and use it to build trigger_details, so the raw
TriggerInfo.config — which can carry worker-supplied metadata or auth
tokens — never surfaces through the explain output.
- inbound: same-topic durable::subscriber peers are sibling consumers,
not upstream feeders. Filter them out of the inbound list; only
non-subscriber triggers on a topic we also consume count as upstream.
topology.rs:
- Aggregate functions_per_worker entries by stable worker id, not
display name. Two workers sharing a name (replicas, or unset name
falling back to id) now render as separate rows.
trace.rs:
- Same node-id collision + label-escape fix as diagram.rs.
- Traversal: same-topic durable::subscriber peers go into a
related_subscribers metadata list on the chain entry rather than being
enqueued as downstream steps. Explicit publish/output relationships
are still enqueued.
workers.rs:
- Consume the owned Vec<WorkerInfo> via into_iter() so json!{} can move
each worker instead of cloning its Vec<String>/Option<String> fields
per-entry. anonymous_count falls out of the captured total.
README.md: add 'text' language to the CLI options fenced block (MD040).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Functions
introspect::functionsintrospect::workersintrospect::triggersintrospect::topologyintrospect::diagramintrospect::healthintrospect::trace_workflowintrospect::explainintrospect::topology_refreshTest plan
cargo test— 20 tests passingcargo check— 0 warningsSummary by CodeRabbit
Release Notes