Repository navigation
Migrate GitHub webhook normalization into github tool - #758
Conversation
github-actions
Bot
added
scope: docs
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the GitHub tool by integrating robust webhook handling capabilities, enabling event-driven workflows. It centralizes GitHub event normalization and enrichment within the tool, and expands the tool's API to support a wider range of interactions, including creating and merging pull requests, managing issue and pull request comments, and retrieving status information. These changes streamline automation and improve the tool's responsiveness to GitHub events. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates GitHub webhook normalization logic into the github tool, adding a handle_webhook action and several other new actions for issues and pull requests. The changes are well-structured, and the addition of webhook capabilities to the tool's manifest is a good practice. I've found one area for improvement in the header parsing logic to make it more robust, aligning with best practices for handling service-specific requirements.
Note: Security Review did not run due to the size of the PR.
| fn header_value<'a>(headers: &'a HashMap<String, String>, key: &str) -> Option<&'a str> { | ||
| headers | ||
| .get(key) | ||
| .or_else(|| headers.get(&key.to_ascii_lowercase())) | ||
| .or_else(|| headers.get(&key.to_ascii_uppercase())) | ||
| .map(String::as_str) |
There was a problem hiding this comment.
The current implementation for header_value is not fully case-insensitive as required by the HTTP specification for header names. It only checks for the exact key, the all-lowercase version, and the all-uppercase version. This could fail to find headers with different casing, such as X-GitHub-Event if the lookup key is x-github-event.
To ensure robust, case-insensitive matching, I suggest iterating through the headers and comparing keys case-insensitively.
headers
.iter()
.find(|(k, _)| k.eq_ignore_ascii_case(key))
.map(|(_, v)| v.as_str())References
- Generic token validation functions, or any functions interacting with service-specific headers, require robust header handling, including case-insensitive matching, to correctly process custom headers and service-specific requirements.
There was a problem hiding this comment.
Fixed in 6c43d04. header_value now iterates all headers with to_ascii_lowercase() comparison, handling all case variants including mixed-case like X-Github-Event.
There was a problem hiding this comment.
Fixed in 6c43d04. header_value now iterates all headers with to_ascii_lowercase() comparison.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Code Review
Overall this is a well-structured PR that follows the existing patterns in the codebase. The webhook normalization logic is clean, the enrichment approach is sensible, and the new CRUD actions are consistent with the existing ones. Good test coverage for the webhook path. A few issues to address:
Issues
1. header_value case handling is fragile (medium)
The header_value helper checks exact key, lowercase, and uppercase -- but HTTP headers are case-insensitive and can be mixed-case (e.g., X-GitHub-Event, x-github-event, X-Github-Event). The current approach misses mixed-case variants. Consider normalizing to lowercase on lookup:
fn header_value<'a>(headers: &'a HashMap<String, String>, key: &str) -> Option<&'a str> {
let lower = key.to_ascii_lowercase();
headers
.iter()
.find(|(k, _)| k.to_ascii_lowercase() == lower)
.map(|(_, v)| v.as_str())
}This is a correctness issue since the runtime delivering the webhook may normalize headers differently.
2. comment_id should be u64, not u32 (medium)
GitHub API uses 64-bit integer IDs for comments (and other resources). The ReplyPullRequestComment::comment_id field is u32, which will silently truncate large IDs. The same concern applies to pr_number and issue_number, though those are less likely to overflow in practice. At minimum, comment_id should be u64 since comment IDs are allocated from a global namespace and are already well into the billions.
3. test_validate_merge_method is a no-op test (low)
This test validates that Rust's contains method works on a literal array -- it doesn't exercise merge_pull_request() at all. Either test the actual function with valid/invalid merge methods, or remove this test.
4. handle_webhook exposed in LLM-facing schema (medium)
The handle_webhook action appears in the tool's JSON schema, meaning the LLM could invoke it directly with crafted webhook payloads, bypassing HMAC verification (which is done at the runtime level before the tool is called). Consider whether this action should be excluded from the schema or whether the tool should validate that it's being called from the webhook dispatch path rather than direct LLM invocation.
5. put_string_normalized overwrites repository and sender objects (low)
GitHub payloads include repository and sender as full JSON objects. The enrichment logic replaces these with just the string values (full_name and login). This is a deliberate design choice for flattening, but downstream consumers lose access to e.g. repository.private, repository.default_branch, sender.avatar_url. Consider using distinct keys like repository_name and sender_login to preserve the original objects.
6. README docs are incomplete
The new actions list_issue_comments, create_issue_comment, list_pull_request_comments, reply_pull_request_comment, get_pull_request_reviews, get_combined_status, and handle_webhook are not documented in the README. Only create_pull_request and merge_pull_request got examples.
Looks Good
- Consistent input validation (
validate_path_segment,validate_input_length,url_encode_path) on all new endpoints - Proper
merge_methodallowlist validation - Webhook capability declaration in capabilities.json with HMAC config
- Good test coverage for event type normalization and enrichment
- Adding
PUTmethod to the HTTP allowlist for the merge endpoint
No blocking issues -- items 1, 2, and 4 are worth addressing before merge. The rest are nice-to-haves.
github-actions
Bot
added
scope: agent
* Add reusable gateway workflow test harness with mock LLM server * Fix clippy issues in workflow harness * Stabilize trace E2E test rig and approval behavior * Address PR review feedback on gateway workflow harness - Extract shared TestChannelHandle into test_channel.rs with name override support, eliminating ~55 lines of duplication between test_rig.rs and gateway_workflow_harness.rs - Remove redundant RoutineEngine creation that was immediately overwritten by Agent::run() - Replace flaky sleep(500ms) with polling loop for routine run count check - Use components.context_manager instead of creating a fresh ContextManager for job tools, ensuring agent and tools share the same instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix import ordering in gateway_workflow_harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review after commits 42e8a6a, e8395b4, 2d8c7db.
Previous feedback status
1. header_value case handling (medium) -- NOT addressed. The function still checks exact, lowercase, and uppercase only. Mixed-case variants like X-Github-Event will be missed. The suggested fix (iterate and compare with to_ascii_lowercase()) is straightforward and would close this gap.
2. comment_id should be u64 (medium) -- NOT addressed. ReplyPullRequestComment::comment_id is still u32. GitHub comment IDs are in the billions range globally. This will silently truncate.
3. test_validate_merge_method no-op test (low) -- The test has been removed entirely. However, test_validate_event_in_create_pr_review (line 830) has the exact same problem: it validates that Rust's contains works on a literal array rather than testing the actual create_pr_review function's event validation. Consider removing or rewriting it.
4. handle_webhook exposed in LLM-facing schema (medium) -- NOT addressed. handle_webhook is still present in the JSON schema. The new commits do not add any guard against direct LLM invocation of this action.
5. put_string_normalized overwrites repository/sender objects (low) -- NOT addressed. Still uses repository and sender keys, overwriting the original objects.
6. README docs incomplete (low) -- PARTIALLY addressed. The README now includes create_pull_request and merge_pull_request examples. The other new actions (list_issue_comments, create_issue_comment, list_pull_request_comments, reply_pull_request_comment, get_pull_request_reviews, get_combined_status, handle_webhook) are still not documented in the README, though they are discoverable via the JSON schema.
New commits review
Commit e8395b4 -- Test rig stabilization:
TestChannelHandleextracted fromtest_rig.rsintotest_channel.rsand made public -- good refactoring, enables reuse by the new gateway harness.- Default
auto_approve_toolschanged fromNonetoSome(true)inTestRigBuilder, and.with_auto_approve_tools(true)added to all test sites. This fixes flaky tests caused by approval prompts blocking the agent loop. Reasonable change. max_tool_callsassertion relaxed from<= 4to<= 8(line 252) -- this warrants a comment explaining why the bound doubled. Is it because auto-approve now lets the agent run more iterations per turn?verify_expectsnow includes failed tool status events in assertion output -- good diagnostic improvement.thread_ops.rschange:auto_approve_toolsconfig now short-circuits the session lock forUnlessAutoApproved. Clean and correct.
Commit 2d8c7db -- Gateway workflow harness:
- Well-structured mock OpenAI server with rule-based response matching. Clean design.
gateway_workflow_integration.rsexercises the full chat-to-webhook-to-routine pipeline. Good coverage.- The
MockGithubWebhookToolin the harness uses.unwrap_or("unknown")(line 447 of harness) -- acceptable in test code. - The harness uses
#[cfg(feature = "libsql")]gating, which is correct for the test infrastructure. - Polling loops with
tokio::time::sleep(100ms)and bounded iteration counts (30, 50) are reasonable for integration tests.
Summary
Items 1, 2, and 4 from the original review remain unaddressed. Items 1 and 2 are correctness issues worth fixing before merge. Item 4 is a design consideration that can be tracked separately if there is a plan for it.
The new test infrastructure is solid and well-organized. The test rig stabilization fixes are sensible.
a367284 to
819db92
Compare
…migration # Conflicts: # Cargo.lock # skills/ironclaw-workflow-orchestrator/SKILL.md # src/agent/routine.rs # src/agent/routine_engine.rs # src/agent/thread_ops.rs # src/channels/web/handlers/routines.rs # src/channels/web/server.rs # src/main.rs # src/tools/builtin/routine.rs # src/tools/registry.rs # src/tools/schema_validator.rs # src/tools/wasm/capabilities.rs # src/tools/wasm/capabilities_schema.rs # src/webhooks/mod.rs # tests/e2e_builtin_tool_coverage.rs # tests/fixtures/llm_traces/tools/routine_system_event_emit.json # tests/fixtures/llm_traces/tools/skill_install_routine_webhook_sim.json # tests/support/test_rig.rs
github-actions
Bot
added
scope: dependencies
There was a problem hiding this comment.
Pull request overview
This PR migrates GitHub webhook event normalization/enrichment into the github WASM tool itself, adds a handle_webhook action for webhook-driven flows, and expands the tool’s GitHub API surface (comments/PR creation/status/merge) while declaring webhook HMAC requirements via tool capabilities.
Changes:
- Add
handle_webhookto the GitHub tool and implement GitHub event type normalization + payload enrichment for emittingsystem_events. - Expand GitHub tool actions (issue comments, PR creation, PR review comments, combined status, merge PR).
- Add webhook HMAC capability metadata and introduce integration test harnesses for gateway + webhook workflows.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools-src/github/src/lib.rs | Adds new GitHub actions plus webhook normalization/enrichment and schema/tests updates. |
| tools-src/github/github-tool.capabilities.json | Declares webhook HMAC verification parameters and allows HTTP PUT for merge. |
| tools-src/github/README.md | Updates docs to reflect new issue/PR capabilities and webhook-driven operations. |
| tests/support/test_rig.rs | Refactors test rig to use TestChannelHandle from test_channel. |
| tests/support/test_channel.rs | Introduces TestChannelHandle wrapper (with optional name override). |
| tests/support/mod.rs | Exposes new support modules for integration harness and mock OpenAI server. |
| tests/support/mock_openai_server.rs | Adds an in-process OpenAI-compatible mock server for integration tests. |
| tests/support/gateway_workflow_harness.rs | Adds a gateway+webhook end-to-end harness (server + webhook ingress + mock tool). |
| tests/gateway_workflow_integration.rs | Adds an integration test that exercises chat → routine creation → webhook ingestion flow. |
| tests/e2e_routine_heartbeat.rs | Adds/adjusts system-event routine trigger tests (but currently introduces compile issues). |
| skills/ironclaw-workflow-orchestrator/SKILL.md | Updates workflow orchestrator guidance to reference webhook endpoint + optional HMAC secret. |
| registry/tools/github.json | Bumps GitHub tool version to 0.2.1. |
| Cargo.lock | Updates dependency lockfile entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ----------------------------------------------------------------------- | ||
|
|
||
| #[tokio::test] | ||
| async fn system_event_trigger_matches_and_filters() { |
There was a problem hiding this comment.
This introduces a second test function named system_event_trigger_matches_and_filters in the same module, which will cause a duplicate definition compile error. Remove this duplicate test or rename it so there is only one function with that name in the module.
| async fn system_event_trigger_matches_and_filters() { | |
| async fn routine_cooldown() { |
There was a problem hiding this comment.
Fixed in 6c43d04 — the duplicate test function was removed.
| let engine = Arc::new(RoutineEngine::new( | ||
| RoutineConfig::default(), | ||
| db.clone(), | ||
| llm, | ||
| ws, | ||
| notify_tx, | ||
| None, | ||
| )); |
There was a problem hiding this comment.
RoutineEngine::new currently requires tools: Arc<ToolRegistry> and safety: Arc<SafetyLayer> parameters (in addition to scheduler). This call site passes only 7 arguments, so it won’t compile; construct minimal ToolRegistry/SafetyLayer (as done elsewhere in this file) and pass them to RoutineEngine::new.
There was a problem hiding this comment.
Fixed in 6c43d04 — the orphaned code with missing args was removed entirely.
| .get(key) | ||
| .or_else(|| headers.get(&key.to_ascii_lowercase())) | ||
| .or_else(|| headers.get(&key.to_ascii_uppercase())) | ||
| .map(String::as_str) | ||
| } | ||
|
|
There was a problem hiding this comment.
header_value() is intended to be case-insensitive, but the current implementation only tries the exact key, all-lowercase, and all-uppercase variants. It will miss common header casing like X-GitHub-Event. Consider normalizing the headers map to lowercase once (when building GitHubWebhookRequest) or doing a true ASCII case-insensitive lookup (e.g., iterate keys and compare with eq_ignore_ascii_case).
| .get(key) | |
| .or_else(|| headers.get(&key.to_ascii_lowercase())) | |
| .or_else(|| headers.get(&key.to_ascii_uppercase())) | |
| .map(String::as_str) | |
| } | |
| .iter() | |
| .find(|(k, _)| k.eq_ignore_ascii_case(key)) | |
| .map(|(_, v)| v.as_str()) | |
| } |
There was a problem hiding this comment.
Fixed in 6c43d04 — header_value() now iterates all keys with eq_ignore_ascii_case (full case-insensitive lookup).
| "action": { "const": "handle_webhook" }, | ||
| "webhook": { | ||
| "type": "object", | ||
| "description": "Generic webhook payload envelope from core runtime" |
There was a problem hiding this comment.
The JSON schema for handle_webhook declares webhook as an unstructured object. Since handle_webhook requires at least webhook.headers and webhook.body_json, consider defining the expected webhook properties in the schema (and marking the required ones) so tool-call generation/validation can catch malformed inputs earlier.
| "description": "Generic webhook payload envelope from core runtime" | |
| "description": "Generic webhook payload envelope from core runtime", | |
| "properties": { | |
| "headers": { | |
| "type": "object", | |
| "description": "HTTP headers for the webhook request", | |
| "additionalProperties": { "type": "string" } | |
| }, | |
| "body_json": { | |
| "type": "object", | |
| "description": "Parsed JSON body of the webhook request" | |
| } | |
| }, | |
| "required": ["headers", "body_json"], | |
| "additionalProperties": true |
There was a problem hiding this comment.
The handle_webhook action has been removed from the LLM-facing schema entirely (6c43d04) — it's only invoked by the webhook ingress runtime, not by the LLM. No schema validation needed.
| #[test] | ||
| fn test_validate_merge_method() { | ||
| let valid = ["merge", "squash", "rebase"]; | ||
| assert!(valid.contains(&"merge")); | ||
| assert!(!valid.contains(&"invalid")); | ||
| } |
There was a problem hiding this comment.
test_validate_merge_method re-implements the list of valid merge methods instead of exercising the actual merge_pull_request validation logic, so it won’t fail if the implementation changes. Consider replacing it with a unit test that calls merge_pull_request (or the validation branch) and asserts that invalid merge_method values are rejected and valid ones are accepted.
There was a problem hiding this comment.
Fixed in 6c43d04 — the no-op test was replaced with test_header_value_case_insensitive which exercises the actual implementation.
- Fix header_value to use fully case-insensitive lookup (iterate with to_ascii_lowercase) instead of checking only exact/lower/upper variants - Change comment_id from u32 to u64 to handle GitHub's billion-range IDs - Remove handle_webhook from LLM-facing JSON schema to prevent direct invocation bypassing HMAC verification - Rename enrichment keys from repository/sender to repository_name/ sender_login to preserve original JSON objects in webhook payloads - Remove put_string_normalized helper (no longer needed) - Replace no-op tests (test_validate_event_in_create_pr_review, test_validate_merge_method) with test_header_value_case_insensitive - Add README docs for 6 undocumented actions (list_issue_comments, create_issue_comment, list_pull_request_comments, reply_pull_request_comment, get_pull_request_reviews, get_combined_status) - Add comment explaining max_tool_calls <= 8 bound in e2e test - Fix gateway workflow harness: add webhook_capability with secret auth to MockGithubWebhookTool, matching staging's hardened webhook security - Fix merge artifacts: remove duplicate test function, orphaned code fragment in e2e_routine_heartbeat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed in 6c43d041. header_value case handling (medium) — Fixed. Now iterates all headers with 2. comment_id u32 → u64 (medium) — Fixed. Changed to 3. No-op tests (low) — Fixed. Removed 4. handle_webhook in LLM schema (medium) — Fixed. Removed from the 5. put_string_normalized overwrites objects (low) — Fixed. Changed to 6. README docs incomplete (low) — Fixed. Added examples for 7. max_tool_calls bound comment — Added explanatory comment for the |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| put_if_missing( | ||
| &mut obj, | ||
| "repository_name", | ||
| payload | ||
| .pointer("/repository/full_name") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| serde_json::Value::String(s.to_string())), | ||
| ); | ||
| put_if_missing( |
There was a problem hiding this comment.
github_enriched_payload adds repository_name, but it leaves the existing repository field as the raw GitHub object. System-event routine filters currently match only top-level keys by string value, and the workflow orchestrator templates use event_filters.repository (string full_name). With the current payload shape, routines filtering on repository will not match webhook events. Consider normalizing so the emitted payload includes a string repository (e.g. repository.full_name) for filtering/back-compat (and preserve the original object under a different key if needed), or update the routine templates/docs to filter on repository_name instead.
There was a problem hiding this comment.
Fixed in 67e0018 — updated SKILL.md event filter docs and all workflow-routines.md templates to use repository_name and sender_login (matching the enriched payload keys).
| put_if_missing( | ||
| &mut obj, | ||
| "pr_number", | ||
| payload.pointer("/pull_request/number").cloned(), | ||
| ); | ||
| put_if_missing( | ||
| &mut obj, |
There was a problem hiding this comment.
For issue_comment webhooks on PRs you emit event types like pr.comment.* (via /issue/pull_request), but github_enriched_payload only sets pr_number from /pull_request/number, which isn’t present for issue_comment payloads. This means PR-comment events may lack pr_number even though they’re classified as PR events. Consider setting pr_number from /issue/number when the issue is a PR (and pr_number is missing).
| put_if_missing( | |
| &mut obj, | |
| "pr_number", | |
| payload.pointer("/pull_request/number").cloned(), | |
| ); | |
| put_if_missing( | |
| &mut obj, | |
| let pr_number = payload | |
| .pointer("/pull_request/number") | |
| .cloned() | |
| .or_else(|| { | |
| // For issue_comment webhooks on PRs, there is no top-level | |
| // /pull_request object, but /issue/pull_request is present and | |
| // /issue/number corresponds to the PR number. | |
| if payload.pointer("/issue/pull_request").is_some() { | |
| payload.pointer("/issue/number").cloned() | |
| } else { | |
| None | |
| } | |
| }); | |
| put_if_missing(&mut obj, "pr_number", pr_number); | |
| put_if_missing( | |
| &mut obj, |
There was a problem hiding this comment.
Fixed in 67e0018 — pr_number now falls back to /issue/number when /pull_request/number is absent and /issue/pull_request exists. Added test_enriched_payload_pr_number_from_issue_comment regression test.
| - GitHub tool authenticated (for issue/PR/comment/status operations). | ||
| - Events are emitted via `event_emit` tool calls (a future HTTP webhook ingestion endpoint is planned but not yet available). | ||
| - GitHub webhook delivery configured to `POST /webhook/tools/github`. | ||
| - Optional webhook HMAC secret configured in secrets store as `github_webhook_secret`. |
There was a problem hiding this comment.
The docs say the GitHub webhook HMAC secret is optional, but the GitHub tool now declares an hmac_secret_name capability, and the generic webhook ingress treats any configured HMAC mechanism as required (it will reject requests if the secret is missing). Either make the secret requirement explicit here, or adjust the tool capability if you truly want webhook auth to be optional.
| - Optional webhook HMAC secret configured in secrets store as `github_webhook_secret`. | |
| - Webhook HMAC secret configured in the secrets store as `github_webhook_secret` (required for GitHub webhook delivery). |
There was a problem hiding this comment.
Fixed in 67e0018 — updated SKILL.md to mark the webhook HMAC secret as required.
| pub mod assertions; | ||
| pub mod cleanup; | ||
| pub mod gateway_workflow_harness; | ||
| pub mod instrumented_llm; | ||
| pub mod metrics; | ||
| pub mod mock_openai_server; |
There was a problem hiding this comment.
tests/support/mod.rs now unconditionally compiles gateway_workflow_harness, but that module depends on the libsql feature (it imports ironclaw::db::libsql::LibSqlBackend). This can break cargo test/cargo clippy --tests for feature sets that don’t include libsql. Consider gating the module export with #[cfg(feature = "libsql")] (similar to how tests/gateway_workflow_integration.rs is gated), or feature-gating the contents of the harness module.
There was a problem hiding this comment.
Fixed in 67e0018 — added #[cfg(feature = "libsql")] gate on gateway_workflow_harness in tests/support/mod.rs. Verified cargo check --tests passes without the libsql feature.
| "display_name": "GitHub", | ||
| "kind": "tool", | ||
| "version": "0.2.0", | ||
| "version": "0.2.1", |
There was a problem hiding this comment.
The registry manifest version was bumped to 0.2.1, but the GitHub tool source still declares 0.2.0 in tools-src/github/Cargo.toml and tools-src/github/github-tool.capabilities.json. If the release/packaging flow expects these versions to match (as they do for other tools), this can cause artifact/version skew. Please align the tool crate/capabilities versions with the registry version (or explain why they intentionally differ).
| "version": "0.2.1", | |
| "version": "0.2.0", |
There was a problem hiding this comment.
Fixed in 67e0018 — bumped Cargo.toml and capabilities.json to 0.2.1 to match the registry manifest.
…, version alignment - Update SKILL.md and workflow-routines.md templates to use `repository_name` and `sender_login` (matching enriched payload field names) - Mark webhook HMAC secret as required in SKILL.md prerequisites - Fall back to `/issue/number` for `pr_number` on issue_comment PR webhooks - Gate `gateway_workflow_harness` module behind `#[cfg(feature = "libsql")]` - Align tool version to 0.2.1 in Cargo.toml and capabilities.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All feedback addressed
All six items from my previous two reviews have been fixed in commit 6c43d04 and subsequent commits:
-
header_value case handling -- Fixed. Now iterates all headers with
to_ascii_lowercase()on both sides, handling any case variant. Regression test added. -
comment_id u32 -> u64 -- Fixed. Changed to u64 throughout (enum, function sig, JSON schema).
-
No-op test functions -- Fixed. Removed the stdlib-testing no-ops, replaced with
test_header_value_case_insensitivethat tests actual behavior. -
handle_webhook in LLM schema -- Fixed. Removed from schema so the LLM can't bypass HMAC verification by calling it directly.
-
put_string_normalized overwriting repository/sender -- Fixed. Enrichment keys renamed to
repository_nameandsender_login, preserving original objects. -
README docs incomplete -- Fixed. All 6 missing actions documented.
Additional improvement in 67e0018: pr_number enrichment now falls back to /issue/number for issue_comment webhooks on PRs where /pull_request/number is absent. Regression test added.
LGTM.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut payload = params | ||
| .pointer("/webhook/body_json") | ||
| .cloned() | ||
| .unwrap_or_else(|| serde_json::json!({})); | ||
| if payload.get("repository").and_then(|v| v.as_str()).is_none() | ||
| && let Some(full_name) = payload | ||
| .pointer("/repository/full_name") | ||
| .and_then(|v| v.as_str()) | ||
| { | ||
| payload["repository"] = serde_json::json!(full_name); | ||
| } | ||
| let event_type = format!( | ||
| "{}.{}", | ||
| if event == "issues" { "issue" } else { event }, | ||
| action | ||
| ); | ||
|
|
There was a problem hiding this comment.
MockGithubWebhookTool normalizes webhook payloads into a top-level repository string, but the real GitHub tool now enriches payloads with repository_name / sender_login (and routines/docs filter on those). This mismatch means the gateway workflow harness test can diverge from production behavior. Consider updating the mock tool’s normalization to populate repository_name (and other normalized fields used by routines) so end-to-end tests validate the same filtering keys.
| let lower = key.to_ascii_lowercase(); | ||
| headers | ||
| .iter() | ||
| .find(|(k, _)| k.to_ascii_lowercase() == lower) |
There was a problem hiding this comment.
header_value() lowercases the search key and then allocates a new lowercase String for every header key during iteration. Since this function is intended to be case-insensitive, using eq_ignore_ascii_case on the existing strings avoids repeated allocations while keeping behavior the same.
| let lower = key.to_ascii_lowercase(); | |
| headers | |
| .iter() | |
| .find(|(k, _)| k.to_ascii_lowercase() == lower) | |
| headers | |
| .iter() | |
| .find(|(k, _)| k.eq_ignore_ascii_case(key)) |
| serde_json::json!({ | ||
| "name": "wf-ci-webhook-demo", | ||
| "description": "CI webhook workflow demo", | ||
| "trigger_type": "system_event", | ||
| "event_source": "github", | ||
| "event_type": "issue.opened", | ||
| "event_filters": {"repository": "nearai/ironclaw"}, | ||
| "action_type": "lightweight", |
There was a problem hiding this comment.
In this integration test, the created routine filters on event_filters.repository, but the GitHub tool’s webhook normalization emits repository_name (and SKILL/docs were updated accordingly). As written, this test can pass while not reflecting the real webhook payload shape. Update the routine creation (and any synthetic event payloads) to use repository_name (and related normalized keys like sender_login) so the test exercises the production flow accurately.
* Add event-triggered routines and workflow skill templates * Add generic host-verified webhook ingress for tools * Migrate GitHub webhook normalization into github tool * Bump github tool registry version * Stabilize trace E2E test rig and approval behavior * Add reusable gateway workflow harness with mock LLM server (nearai#762) * Add reusable gateway workflow test harness with mock LLM server * Fix clippy issues in workflow harness * Stabilize trace E2E test rig and approval behavior * Address PR review feedback on gateway workflow harness - Extract shared TestChannelHandle into test_channel.rs with name override support, eliminating ~55 lines of duplication between test_rig.rs and gateway_workflow_harness.rs - Remove redundant RoutineEngine creation that was immediately overwritten by Agent::run() - Replace flaky sleep(500ms) with polling loop for routine run count check - Use components.context_manager instead of creating a fresh ContextManager for job tools, ensuring agent and tools share the same instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix import ordering in gateway_workflow_harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Address PR nearai#758 review feedback - Fix header_value to use fully case-insensitive lookup (iterate with to_ascii_lowercase) instead of checking only exact/lower/upper variants - Change comment_id from u32 to u64 to handle GitHub's billion-range IDs - Remove handle_webhook from LLM-facing JSON schema to prevent direct invocation bypassing HMAC verification - Rename enrichment keys from repository/sender to repository_name/ sender_login to preserve original JSON objects in webhook payloads - Remove put_string_normalized helper (no longer needed) - Replace no-op tests (test_validate_event_in_create_pr_review, test_validate_merge_method) with test_header_value_case_insensitive - Add README docs for 6 undocumented actions (list_issue_comments, create_issue_comment, list_pull_request_comments, reply_pull_request_comment, get_pull_request_reviews, get_combined_status) - Add comment explaining max_tool_calls <= 8 bound in e2e test - Fix gateway workflow harness: add webhook_capability with secret auth to MockGithubWebhookTool, matching staging's hardened webhook security - Fix merge artifacts: remove duplicate test function, orphaned code fragment in e2e_routine_heartbeat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix formatting in gateway workflow harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address Copilot review: filter keys, pr_number fallback, feature gate, version alignment - Update SKILL.md and workflow-routines.md templates to use `repository_name` and `sender_login` (matching enriched payload field names) - Mark webhook HMAC secret as required in SKILL.md prerequisites - Fall back to `/issue/number` for `pr_number` on issue_comment PR webhooks - Gate `gateway_workflow_harness` module behind `#[cfg(feature = "libsql")]` - Align tool version to 0.2.1 in Cargo.toml and capabilities.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Add event-triggered routines and workflow skill templates * Add generic host-verified webhook ingress for tools * Migrate GitHub webhook normalization into github tool * Bump github tool registry version * Stabilize trace E2E test rig and approval behavior * Add reusable gateway workflow harness with mock LLM server (nearai#762) * Add reusable gateway workflow test harness with mock LLM server * Fix clippy issues in workflow harness * Stabilize trace E2E test rig and approval behavior * Address PR review feedback on gateway workflow harness - Extract shared TestChannelHandle into test_channel.rs with name override support, eliminating ~55 lines of duplication between test_rig.rs and gateway_workflow_harness.rs - Remove redundant RoutineEngine creation that was immediately overwritten by Agent::run() - Replace flaky sleep(500ms) with polling loop for routine run count check - Use components.context_manager instead of creating a fresh ContextManager for job tools, ensuring agent and tools share the same instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix import ordering in gateway_workflow_harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Address PR nearai#758 review feedback - Fix header_value to use fully case-insensitive lookup (iterate with to_ascii_lowercase) instead of checking only exact/lower/upper variants - Change comment_id from u32 to u64 to handle GitHub's billion-range IDs - Remove handle_webhook from LLM-facing JSON schema to prevent direct invocation bypassing HMAC verification - Rename enrichment keys from repository/sender to repository_name/ sender_login to preserve original JSON objects in webhook payloads - Remove put_string_normalized helper (no longer needed) - Replace no-op tests (test_validate_event_in_create_pr_review, test_validate_merge_method) with test_header_value_case_insensitive - Add README docs for 6 undocumented actions (list_issue_comments, create_issue_comment, list_pull_request_comments, reply_pull_request_comment, get_pull_request_reviews, get_combined_status) - Add comment explaining max_tool_calls <= 8 bound in e2e test - Fix gateway workflow harness: add webhook_capability with secret auth to MockGithubWebhookTool, matching staging's hardened webhook security - Fix merge artifacts: remove duplicate test function, orphaned code fragment in e2e_routine_heartbeat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix formatting in gateway workflow harness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address Copilot review: filter keys, pr_number fallback, feature gate, version alignment - Update SKILL.md and workflow-routines.md templates to use `repository_name` and `sender_login` (matching enriched payload field names) - Mark webhook HMAC secret as required in SKILL.md prerequisites - Fall back to `/issue/number` for `pr_number` on issue_comment PR webhooks - Gate `gateway_workflow_harness` module behind `#[cfg(feature = "libsql")]` - Align tool version to 0.2.1 in Cargo.toml and capabilities.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Validation