fix(github): reduce PR-prioritization tool churn - #6953
serrrfirat wants to merge 14 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-6953 environment in ironclaw-ci-preview
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds regression-promotion guidance, suppresses unsafe large-result previews while preserving continuation metadata, updates GitHub self-scoped search contracts, and extends PR test-plan routing for guidance and extension assets. ChangesRegression promotion workflow
Safe result preview continuation
GitHub self-scoped search contracts
PR test-plan path routing
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
github-actions
Bot
added
size: L
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 Review · PR #6953
Execution result is invalid The structured result could not be verified. Automatic · PR opened · attempt 1 of 3 · failed after 3m 6s Failure details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/promote-run-regression/SKILL.md:
- Around line 116-117: Update the final fixture check around the rg command to
avoid printing matching JSON content; use rg -l or a structured jq validation
that reports only the affected filename and failing key while still checking
"_review", PENDING_REPLAY_COMMIT, and PR_NUMBER.
In `@crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs`:
- Around line 1655-1748: Add a small table-driven caller-level test alongside
search_issues_pull_requests_compacts_items_and_preserves_envelope_and_errors
that invokes execute_inner for malformed providers: a non-object search body, a
search object missing its items array, and a list_pull_requests response
containing a non-object item. Set each response through
test_support::set_response and assert execute_inner returns
github_api_invalid_response, while preserving the existing happy-path and
provider-error coverage.
In `@crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs`:
- Around line 351-357: Extend the list pull requests test around the existing
requests[0] assertions to verify the request has an empty body, the expected
applied network policy, and the injected authorization header, matching the
search-path contract assertions. Reuse github_wasm_services_for_test! in the
search test setup, retaining local policy and slot_handle bindings so its
existing assertions remain valid.
In `@tests/reborn_qa_recorded_behavior.rs`:
- Around line 427-443: Update the test around the github.get_authenticated_user
and github.search_issues_pull_requests assertions to capture the
authenticated-user result and assert that its login is the value used for the
search author. Replace the standalone literal author check with a comparison
against the returned login while preserving the existing owner, repo, state,
type, and call-sequence assertions.
- Around line 448-452: Strengthen the assertion in the priority-list workflow
around final_text_reply so it verifies the caller-visible ranked-list outcome,
not merely the presence of “priority.” Assert stable expected entries from the
fixture and the expected list structure, preserving the existing final reply
extraction and failure context.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ec9df7b-3a2c-4d47-a382-d1704d084191
⛔ Files ignored due to path filters (2)
crates/ironclaw_first_party_extensions/assets/github/wasm/github_tool.wasmis excluded by!**/*.wasm,!**/*.wasmtests/fixtures/llm_traces/reborn_qa/github_open_pr_priority.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (18)
.claude/skills/promote-run-regression/SKILL.mdAGENTS.mdcrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_first_party_extensions/assets/github/manifest.tomlcrates/ironclaw_first_party_extensions/assets/github/prompts/github/get_authenticated_user.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/list_pull_requests.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues_pull_requests.mdcrates/ironclaw_first_party_extensions/assets/github/wasm-src/src/api/pulls.rscrates/ironclaw_first_party_extensions/assets/github/wasm-src/src/api/search.rscrates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rscrates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rscrates/ironclaw_host_api/src/resolution.rscrates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rscrates/ironclaw_reborn_composition/src/runtime/tests/core.rscrates/ironclaw_turns/src/run_profile/resolution.rsdocs/internal/testing-playbook.mdtests/reborn_qa_recorded_behavior.rs
| rg -n '"_review"|PENDING_REPLAY_COMMIT|PR_NUMBER' \ | ||
| tests/fixtures/llm_traces/reborn_qa --glob '*.json' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not print raw JSON lines during the final fixture check.
rg -n prints the complete matching JSON line. A minified fixture can expose the entire artifact in terminal logs. Use rg -l or a structured jq check that reports only the file and failing key.
As per coding guidelines, never commit secrets or PII.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/promote-run-regression/SKILL.md around lines 116 - 117,
Update the final fixture check around the rg command to avoid printing matching
JSON content; use rg -l or a structured jq validation that reports only the
affected filename and failing key while still checking "_review",
PENDING_REPLAY_COMMIT, and PR_NUMBER.
Source: Coding guidelines
| #[test] | ||
| fn search_issues_pull_requests_compacts_items_and_preserves_envelope_and_errors() { | ||
| let large_body = "x".repeat(32 * 1024); | ||
| let provider_items = (0..100) | ||
| .map(|index| { | ||
| json!({ | ||
| "id": 20_000 + index, | ||
| "node_id": format!("I_{index}"), | ||
| "number": 6_000 + index, | ||
| "title": format!("Search result {index}"), | ||
| "body": large_body, | ||
| "state": "open", | ||
| "state_reason": null, | ||
| "locked": false, | ||
| "draft": index % 2 == 0, | ||
| "html_url": format!("https://github.com/nearai/ironclaw/pull/{}", 6_000 + index), | ||
| "repository_url": "https://api.github.com/repos/nearai/ironclaw", | ||
| "user": {"login": format!("author-{index}"), "avatar_url": "https://example.test/avatar"}, | ||
| "labels": [{"name": "bug", "description": large_body}], | ||
| "assignees": [{"login": "owner", "avatar_url": "https://example.test/avatar"}], | ||
| "milestone": {"title": "next", "description": large_body}, | ||
| "comments": 17, | ||
| "created_at": "2026-07-01T00:00:00Z", | ||
| "updated_at": "2026-07-31T00:00:00Z", | ||
| "closed_at": null, | ||
| "author_association": "MEMBER", | ||
| "pull_request": { | ||
| "url": format!("https://api.github.com/repos/nearai/ironclaw/pulls/{}", 6_000 + index), | ||
| "html_url": format!("https://github.com/nearai/ironclaw/pull/{}", 6_000 + index), | ||
| "diff_url": "https://example.test/large.diff", | ||
| "patch_url": "https://example.test/large.patch" | ||
| }, | ||
| "reactions": {"total_count": 999, "url": "https://api.github.com/large"}, | ||
| "performed_via_github_app": {"description": large_body}, | ||
| "score": 1.0 | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| let provider_output = json!({ | ||
| "total_count": 12_345, | ||
| "incomplete_results": true, | ||
| "items": provider_items | ||
| }) | ||
| .to_string(); | ||
| assert!(provider_output.len() > 6 * 1024 * 1024); | ||
| test_support::set_response(Ok(provider_output)); | ||
|
|
||
| let output = execute_inner( | ||
| r#"{"repo":"nearai/ironclaw","type":"pr","page":4,"limit":100}"#, | ||
| Some(r#"{"capability_id":"github.search_issues_pull_requests"}"#), | ||
| ) | ||
| .expect("github.search_issues_pull_requests should compact the provider response"); | ||
|
|
||
| assert!( | ||
| output.len() < 100 * 1024, | ||
| "100 compact search results should stay model-useful, got {} bytes", | ||
| output.len() | ||
| ); | ||
| let parsed: serde_json::Value = | ||
| serde_json::from_str(&output).expect("compact search output should be JSON"); | ||
| assert_eq!(parsed["total_count"], 12_345); | ||
| assert_eq!(parsed["incomplete_results"], true); | ||
| assert_eq!(parsed["items"].as_array().map(Vec::len), Some(100)); | ||
| assert_eq!(parsed["items"][0]["user"], json!({"login": "author-0"})); | ||
| assert_eq!(parsed["items"][0]["labels"], json!([{"name": "bug"}])); | ||
| assert_eq!( | ||
| parsed["items"][0]["pull_request"], | ||
| json!({ | ||
| "url": "https://api.github.com/repos/nearai/ironclaw/pulls/6000", | ||
| "html_url": "https://github.com/nearai/ironclaw/pull/6000" | ||
| }) | ||
| ); | ||
| assert!( | ||
| parsed["items"][0].get("body").is_none() | ||
| && parsed["items"][0].get("reactions").is_none() | ||
| && parsed["items"][0].get("performed_via_github_app").is_none(), | ||
| "large detail must remain available through github.get_issue or github.get_pull_request" | ||
| ); | ||
| assert_eq!( | ||
| test_support::requests()[0].path, | ||
| "/search/issues?q=repo%3Anearai%2Fironclaw%20is%3Apr&per_page=100&page=4" | ||
| ); | ||
|
|
||
| test_support::set_response(Err("github_api_error_status_503".to_string())); | ||
| assert_eq!( | ||
| execute_inner( | ||
| r#"{"repo":"nearai/ironclaw","type":"pr"}"#, | ||
| Some(r#"{"capability_id":"github.search_issues_pull_requests"}"#), | ||
| ) | ||
| .unwrap_err(), | ||
| "github_api_error_status_503", | ||
| "provider errors must pass through the compacting response path" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add caller-level coverage for the github_api_invalid_response branches.
The happy path and the provider-error passthrough are both covered. The rejection branches in response.rs are not. validate_page_size and the shape checks (compact_issue_search on a non-object body, a missing items array, or a non-object item) all collapse to one model-visible code, github_api_invalid_response. Add a small table-driven case through execute_inner so a future change to the compactor cannot silently turn a valid provider page into that error.
💚 Suggested additional test
#[test]
fn search_and_list_reject_malformed_provider_shapes() {
for (capability, params, provider) in [
(
"github.search_issues_pull_requests",
r#"{"repo":"nearai/ironclaw","type":"pr"}"#,
json!([]).to_string(),
),
(
"github.search_issues_pull_requests",
r#"{"repo":"nearai/ironclaw","type":"pr"}"#,
json!({"total_count": 1}).to_string(),
),
(
"github.list_pull_requests",
r#"{"owner":"nearai","repo":"ironclaw"}"#,
json!(["not-an-object"]).to_string(),
),
] {
test_support::set_response(Ok(provider));
assert_eq!(
execute_inner(
params,
Some(&format!(r#"{{"capability_id":"{capability}"}}"#))
)
.unwrap_err(),
"github_api_invalid_response",
"{capability} must reject malformed provider shapes"
);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs`
around lines 1655 - 1748, Add a small table-driven caller-level test alongside
search_issues_pull_requests_compacts_items_and_preserves_envelope_and_errors
that invokes execute_inner for malformed providers: a non-object search body, a
search object missing its items array, and a list_pull_requests response
containing a non-object item. Set each response through
test_support::set_response and assert execute_inner returns
github_api_invalid_response, while preserving the existing happy-path and
provider-error coverage.
| let requests = network.requests(); | ||
| assert_eq!(requests.len(), 1); | ||
| assert_eq!( | ||
| requests[0].url, | ||
| "https://api.github.com/repos/nearai/ironclaw/pulls?state=open&per_page=30&page=3" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert credential injection and network policy on the list path too.
github.list_pull_requests declares effects = ["network", "use_secret"]. The search test at lines 249-262 asserts the method, the empty body, the applied policy, and the injected authorization header. This test asserts only the request count and the URL. A regression that dropped the InjectCredentialAccountOnce obligation or the ApplyNetworkPolicy obligation for the list capability would still pass.
🔒️ Proposed fix
let requests = network.requests();
assert_eq!(requests.len(), 1);
+ assert_eq!(requests[0].method, NetworkMethod::Get);
+ assert_eq!(requests[0].body, Vec::<u8>::new());
+ assert_eq!(requests[0].policy, github_policy());
+ assert_eq!(
+ requests[0]
+ .headers
+ .iter()
+ .find(|(name, _)| name == "authorization"),
+ Some(&(
+ "authorization".to_string(),
+ "Bearer ghp_fake_fixture_token".to_string(),
+ ))
+ );
assert_eq!(
requests[0].url,
"https://api.github.com/repos/nearai/ironclaw/pulls?state=open&per_page=30&page=3"
);Separately: the search test builds HostRuntimeServices inline at lines 181-208 while this test uses the new github_wasm_services_for_test! macro. Reusing the macro in the search test would remove that duplication, though the search test needs local policy and slot_handle bindings for its assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let requests = network.requests(); | |
| assert_eq!(requests.len(), 1); | |
| assert_eq!( | |
| requests[0].url, | |
| "https://api.github.com/repos/nearai/ironclaw/pulls?state=open&per_page=30&page=3" | |
| ); | |
| } | |
| let requests = network.requests(); | |
| assert_eq!(requests.len(), 1); | |
| assert_eq!(requests[0].method, NetworkMethod::Get); | |
| assert_eq!(requests[0].body, Vec::<u8>::new()); | |
| assert_eq!(requests[0].policy, github_policy()); | |
| assert_eq!( | |
| requests[0] | |
| .headers | |
| .iter() | |
| .find(|(name, _)| name == "authorization"), | |
| Some(&( | |
| "authorization".to_string(), | |
| "Bearer ghp_fake_fixture_token".to_string(), | |
| )) | |
| ); | |
| assert_eq!( | |
| requests[0].url, | |
| "https://api.github.com/repos/nearai/ironclaw/pulls?state=open&per_page=30&page=3" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs` around
lines 351 - 357, Extend the list pull requests test around the existing
requests[0] assertions to verify the request has an empty body, the expected
applied network policy, and the injected authorization header, matching the
search-path contract assertions. Reuse github_wasm_services_for_test! in the
search test setup, retaining local policy and slot_handle bindings so its
existing assertions remain valid.
| assert_tool_sequence( | ||
| &trace, | ||
| &[ | ||
| "github.get_authenticated_user", | ||
| "github.search_issues_pull_requests", | ||
| ], | ||
| ); | ||
| assert_tool_called_with( | ||
| &trace, | ||
| "github.search_issues_pull_requests", | ||
| &[ | ||
| r#""owner":"nearai""#, | ||
| r#""repo":"ironclaw""#, | ||
| r#""author":"fixture-user""#, | ||
| r#""state":"open""#, | ||
| r#""type":"pr""#, | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert that the search author comes from the authenticated-user result.
The test checks call order and a literal "author":"fixture-user", but it does not compare that value with the result of github.get_authenticated_user. A regression can ignore the lookup and still pass this test. Assert that the returned login equals the search author.
Based on the GitHub search contract, the authenticated login must populate author.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_qa_recorded_behavior.rs` around lines 427 - 443, Update the test
around the github.get_authenticated_user and github.search_issues_pull_requests
assertions to capture the authenticated-user result and assert that its login is
the value used for the search author. Replace the standalone literal author
check with a comparison against the returned login while preserving the existing
owner, repo, state, type, and call-sequence assertions.
| let reply = final_text_reply(&trace).expect("priority-list phrase should finalize a reply"); | ||
| assert!( | ||
| reply.to_ascii_lowercase().contains("priority"), | ||
| "priority-list workflow should end with a priority list" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a positive priority-list assertion.
reply.to_ascii_lowercase().contains("priority") also accepts a refusal or unrelated text. Assert a stable positive outcome, such as expected ranked entries and list structure from the fixture.
Based on .claude/skills/promote-run-regression/SKILL.md, the regression must assert the caller-visible final outcome.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_qa_recorded_behavior.rs` around lines 448 - 452, Strengthen the
assertion in the priority-list workflow around final_text_reply so it verifies
the caller-visible ranked-list outcome, not merely the presence of “priority.”
Assert stable expected entries from the fixture and the expected list structure,
preserving the existing final reply extraction and failure context.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs (1)
569-677: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the PR cover
list_pull_requestswith the real WASM HTTP limit.
list_pull_requestscallsgithub_request(...).and_then(compact_pull_request_list), butgithub_requestmaps host body-size failures togithub_api_body_limit. This test usestest_support::set_response, which bypasses the production WASM egress path and can let compaction run when host transport would reject a large body. Add host-runtime/real-transport coverage for the host response-limit ordering so CLAUDE.md’s “Test through the caller” invariant covers this egress gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs` around lines 569 - 677, Extend coverage around list_pull_requests and github_request using the host-runtime real HTTP transport rather than test_support::set_response. Configure a response exceeding the WASM host body limit, invoke list_pull_requests through its normal caller path, and assert github_api_body_limit is returned before compact_pull_request_list can run. Preserve a separate successful compact-response assertion if needed, ensuring the test validates the production egress gate and caller ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues_pull_requests.md`:
- Line 9: Update the search prompt description around compact_search_item to
stop promising a standalone type marker and instead describe the optional
pull_request object as the discriminator between pull requests and issues. Keep
the documented fields aligned with the schema implemented by compact_search_item
and its tests.
---
Outside diff comments:
In `@crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs`:
- Around line 569-677: Extend coverage around list_pull_requests and
github_request using the host-runtime real HTTP transport rather than
test_support::set_response. Configure a response exceeding the WASM host body
limit, invoke list_pull_requests through its normal caller path, and assert
github_api_body_limit is returned before compact_pull_request_list can run.
Preserve a separate successful compact-response assertion if needed, ensuring
the test validates the production egress gate and caller ordering.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db3c0b82-8853-4fca-bcce-0d0083e6efe8
⛔ Files ignored due to path filters (1)
tests/fixtures/llm_traces/reborn_qa/github_open_pr_priority.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (6)
crates/ironclaw_first_party_extensions/assets/github/prompts/github/get_authenticated_user.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/list_pull_requests.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues.mdcrates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues_pull_requests.mdcrates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rstests/reborn_qa_recorded_behavior.rs
|
|
||
| Prefer the structured `owner`, `repo`, `author`, `assignee`, `involves`, `state`, and `type` fields over duplicating those qualifiers in `query`. | ||
|
|
||
| The result keeps GitHub's `total_count`, `incomplete_results`, and `items` search envelope while returning compact item summaries with the repository URL, number, title, type marker, state/draft status, URL, author, labels, assignees, milestone, comment count, timestamps, and score. Use `page` and `limit` to continue through results. For bodies or other full detail, call `github.get_pull_request` for pull request items or `github.get_issue` for issue items. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the prompt with the compact search schema.
Line 9 promises a standalone “type marker”, but response.rs::compact_search_item does not copy a type field or add another marker. It preserves the optional pull_request object instead. Either add an explicit type field and test it, or describe pull_request presence as the discriminator.
As per path instructions, documentation promising guarantees must match code and tests.
Suggested prompt fix
-... number, title, type marker, state/draft status, URL, author, ...
+... number, title, state/draft status, URL, author, ...
+For pull requests, use the returned `pull_request` metadata to identify the result type.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The result keeps GitHub's `total_count`, `incomplete_results`, and `items` search envelope while returning compact item summaries with the repository URL, number, title, type marker, state/draft status, URL, author, labels, assignees, milestone, comment count, timestamps, and score. Use `page` and `limit` to continue through results. For bodies or other full detail, call `github.get_pull_request` for pull request items or `github.get_issue` for issue items. | |
| The result keeps GitHub's `total_count`, `incomplete_results`, and `items` search envelope while returning compact item summaries with the repository URL, number, title, state/draft status, URL, author, labels, assignees, milestone, comment count, timestamps, and score. Use `page` and `limit` to continue through results. For pull requests, use the returned `pull_request` metadata to identify the result type. For bodies or other full detail, call `github.get_pull_request` for pull request items or `github.get_issue` for issue items. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/ironclaw_first_party_extensions/assets/github/prompts/github/search_issues_pull_requests.md`
at line 9, Update the search prompt description around compact_search_item to
stop promising a standalone type marker and instead describe the optional
pull_request object as the discriminator between pull requests and issues. Keep
the documented fields aligned with the schema implemented by compact_search_item
and its tests.
Source: Path instructions
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Reduce PR-prioritization tool churn by compacting GitHub list/search outputs, routing self-scoped searches via @me, and preserving durable continuation refs when preview text is suppressed.
Shape: primary mode normal; no modifiers. XL PR (992+/54−, 20 files) packetized into 4 buckets (production/tests/config/docs). Local-git diff against main.
Coverage:
Stats: 6 findings (from 6 raw, 6 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: bugs (stalled). Receipt-only-but-noted: local-patterns, approach. Body-only: 0 (all 6 resolve to valid diff lines).
Reviewers failed / partial:
bugs— stalled at 112 turns reading the test packet, no receipt emitted. Its lens (production error-class behavior) was partially recovered: thegithub_api_invalid_responseerror-class finding below was verified by the orchestrator againstlib.rs:guest_error_kindand is flaggedlocal-patterns+ cross-noted. A dedicated bugs re-review is recommended.local-patterns,approach— completed review but emitted thedelegateagent'sacceptance-reportscaffold instead of the strict{coverage, findings}receipt. local-patterns findings were recovered from its narrative and verified; approach reported 0 findings in its narrative.
Tests
-
Medium compact_pull_request_list error paths untested (
crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:5-13, confidence 80) — anchor:crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:5
New compact_pull_request_list returns github_api_invalid_response via?/ok_or_else for malformed JSON, non-array top-level response, item_count > MAX_PAGE_ITEMS (validate_page_size), item not an object, and wrong-type nested fields (user/labels/head/base not object/array). Only the happy 100-item path and the upstream provider Err pass-through are exercised; none of the response.rs-authored invalid_response_error branches have a test. GitHub pagination drift or proxy-injected malformed payloads could hit these paths silently.
Fix: tests::list_pull_requests_compaction_rejects_malformed_provider_response covering malformed JSON, non-array body, 101-item oversized page, item-not-object, and head/labels/user wrong-type fields returning github_api_invalid_response -
Medium compact_issue_search envelope error paths untested (
crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:16-28, confidence 80) — anchor:crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:16
New compact_issue_search returns github_api_invalid_response when the response is not a JSON object, lacks the items array, items is not an array, item_count > 100, or search items have wrong-type nested fields (user/labels/milestone/pull_request shape). The compact path is only exercised on the happy 100-item fixture; the response.rs-authored invalid branches lack tests.
Fix: tests::search_issues_compaction_rejects_malformed_provider_response covering non-object body, missing items, 101-item oversized page, and wrong-type nested fields returning github_api_invalid_response
Local Patterns
-
Medium New github_api_invalid_response code erased to generic operation_failed (
crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:178-179, confidence 78) — anchor:crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/lib.rs:99
New error codegithub_api_invalid_responseinvented in response.rs:178 diverges from siblinggithub_api_error_status_{N}convention in request.rs:42,45. It is not enumerated in guest_error_kind (lib.rs:90-99), so it falls through the_ => "operation_failed"default arm. A malformed/oversized/wrong-type GitHub response — a distinct provider-output contract violation — is reported to the host/driver under the generic operation_failed class, erasing its stable class semantics and making it indistinguishable from unrelated host failures. AGENTS.md trust-boundary checklist requires driver/operator-visible errors have stable class semantics.
Fix: Add an explicit arm in guest_error_kind mappinggithub_api_invalid_responseto a stable class such asinput(malformed provider output / contract violation), and keep the name under thegithub_api_*sibling prefix. -
Low Stale TRUNCATED-preview wording in continuation doc comment (
crates/ironclaw_turns/src/run_profile/resolution.rs:544-546, confidence 72) — anchor:crates/ironclaw_turns/src/run_profile/resolution.rs:544
Doc comment saysResultPreviewMetacarriesTRUNCATED-preview continuation info, but the behavior change preserves continuation metadata when a preview is rejected (not truncated). Sibling host_api/resolution.rs:449-454 was reworded to say 'remains present when an unsafe preview is suppressed'; this turns copy was not reconciled. A reader mis-scopes ResultPreviewMeta as truncated-only.
Fix: Reconcile the turns/resolution.rs:544-546 doc sentence with the host_api rewording — replace 'TRUNCATED-preview continuation info' with 'continuation metadata, preserved even when a preview is suppressed'. -
Nit response.rs module lacks doc comment (
crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:1-1, confidence 65) — anchor:crates/ironclaw_first_party_extensions/assets/github/wasm-src/src/response.rs:1
New response.rs module performs non-obvious compaction/projection of GitHub PR/search results but has no module-level doc comment. Sibling validation.rs:7-8 documents exported items. A short//!module doc would help maintainers understand the compaction contract and the 100-item cap intent.
Fix: Add a//!module doc comment at response.rs:1 describing compaction, field projection, and the MAX_PAGE_ITEMS cap. -
Nit Test helper rename refs_preview->outcome_refs drifts from sibling naming (
crates/ironclaw_turns/src/run_profile/resolution.rs:1597-1597, confidence 55) — anchor:crates/ironclaw_turns/src/run_profile/resolution.rs:1599
Test helper renamed from refs_preview to outcome_refs after a scope change. Sibling completion-test naming elsewhere uses refs_preview-shaped verbs. Harmless local rename but a reader grepping for refs_preview may miss the renamed helper.
Fix: Optional: keep a brief alias or align sibling completion-test naming to outcome_refs.
Security / Bugs / Performance / Conventions / Maintainability / Approach
All returned 0 findings (security/performance/conventions/maintainability/approach). Bugs lens not fully covered — see partial-coverage note. Note: the github_api_invalid_response error-class erasure flagged under Local Patterns is also a bugs-class concern (production error semantics) and was the most likely hit a completed bugs review would have surfaced.
Note on coverage: This review ran on the delegate builtin subagent which injected an acceptance-report scaffold that caused several reviewers to emit narrative instead of the strict receipt contract. Findings marked as reconstructed were verified by the orchestrator against the live worktree at head_sha. Recommend re-running with a stricter reviewer agent for the bugs lens.
| serde_json::to_string(value).map_err(|_| invalid_response_error()) | ||
| } | ||
|
|
||
| fn invalid_response_error() -> String { |
There was a problem hiding this comment.
Medium — New github_api_invalid_response code erased to generic operation_failed.
New error code github_api_invalid_response invented in response.rs:178 diverges from sibling github_api_error_status_{N} convention in request.rs:42,45. It is not enumerated in guest_error_kind (lib.rs:90-99), so it falls through the _ => "operation_failed" default arm. A malformed/oversized/wrong-type GitHub response — a distinct provider-output contract violation — is reported to the host/driver under the generic operation_failed class, erasing its stable class semantics and making it indistinguishable from unrelated host failures. AGENTS.md trust-boundary checklist requires driver/operator-visible errors have stable class semantics.
Fix: Add an explicit arm in guest_error_kind mapping github_api_invalid_response to a stable class such as input (malformed provider output / contract violation), and keep the name under the github_api_* sibling prefix.
"github_api_invalid_response" => "input",| /// referenced result ref, full byte size, next offset, and JSON-array element | ||
| /// count, so the model reads the full result. Detail kinds other than | ||
| /// `ResultReference` have no inline content. | ||
| /// count, so the model reads the full result. This metadata is preserved even |
There was a problem hiding this comment.
Low — Stale TRUNCATED-preview wording in continuation doc comment.
Doc comment says ResultPreviewMeta carries TRUNCATED-preview continuation info, but the behavior change preserves continuation metadata when a preview is rejected (not truncated). Sibling host_api/resolution.rs:449-454 was reworded to say 'remains present when an unsafe preview is suppressed'; this turns copy was not reconciled. A reader mis-scopes ResultPreviewMeta as truncated-only.
Fix: Reconcile the turns/resolution.rs:544-546 doc sentence with the host_api rewording — replace 'TRUNCATED-preview continuation info' with 'continuation metadata, preserved even when a preview is suppressed'.
/// the model reads the full result. This metadata is preserved even when
| @@ -0,0 +1,180 @@ | |||
| use serde_json::{Map, Value}; | |||
There was a problem hiding this comment.
Nit — response.rs module lacks doc comment.
New response.rs module performs non-obvious compaction/projection of GitHub PR/search results but has no module-level doc comment. Sibling validation.rs:7-8 documents exported items. A short //! module doc would help maintainers understand the compaction contract and the 100-item cap intent.
Fix: Add a //! module doc comment at response.rs:1 describing compaction, field projection, and the MAX_PAGE_ITEMS cap.
//! Compacts GitHub PR-list and issue/PR-search provider responses before they
| other => panic!("expected GateRecord::Resource, got {other:?}"), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit — Test helper rename refs_preview->outcome_refs drifts from sibling naming.
Test helper renamed from refs_preview to outcome_refs after a scope change. Sibling completion-test naming elsewhere uses refs_preview-shaped verbs. Harmless local rename but a reader grepping for refs_preview may miss the renamed helper.
Fix: Optional: keep a brief alias or align sibling completion-test naming to outcome_refs.
// outcome_refs (was refs_preview; scope expanded to preserve continuation meta)|
|
||
| const MAX_PAGE_ITEMS: usize = 100; | ||
|
|
||
| pub(crate) fn compact_pull_request_list(response: String) -> Result<String, String> { |
There was a problem hiding this comment.
Medium — compact_pull_request_list error paths untested.
New compact_pull_request_list returns github_api_invalid_response via ?/ok_or_else for malformed JSON, non-array top-level response, item_count > MAX_PAGE_ITEMS (validate_page_size), item not an object, and wrong-type nested fields (user/labels/head/base not object/array). Only the happy 100-item path and the upstream provider Err pass-through are exercised; none of the response.rs-authored invalid_response_error branches have a test. GitHub pagination drift or proxy-injected malformed payloads could hit these paths silently.
Fix: tests::list_pull_requests_compaction_rejects_malformed_provider_response covering malformed JSON, non-array body, 101-item oversized page, item-not-object, and head/labels/user wrong-type fields returning github_api_invalid_response
// set_response(Ok("not json".into())); assert!(matches!(execute_inner(...), Err(e) if e.contains("github_api_invalid_response")))| serialize(&compact) | ||
| } | ||
|
|
||
| pub(crate) fn compact_issue_search(response: String) -> Result<String, String> { |
There was a problem hiding this comment.
Medium — compact_issue_search envelope error paths untested.
New compact_issue_search returns github_api_invalid_response when the response is not a JSON object, lacks the items array, items is not an array, item_count > 100, or search items have wrong-type nested fields (user/labels/milestone/pull_request shape). The compact path is only exercised on the happy 100-item fixture; the response.rs-authored invalid branches lack tests.
Fix: tests::search_issues_compaction_rejects_malformed_provider_response covering non-object body, missing items, 101-item oversized page, and wrong-type nested fields returning github_api_invalid_response
// set_response(Ok("[]".into())); assert!(matches!(execute_inner(...), Err(e) if e.contains("github_api_invalid_response")))Resolve conflicts from the extensions colocation (WS2) refactor: - github wasm-src lib.rs: keep self-qualifier search test on new path - response.rs: identical both sides, accept at new path - list_pull_requests.md / search_issues_pull_requests.md: keep PR's @me guidance at new extensions path - github_tool.wasm: drop old ironclaw_first_party_extensions path, keep new extensions path (main deleted old, PR modified bytes) - resolution.rs: keep main's suppressed_array meta test Local validation: - cargo test -p ironclaw_loop_contracts --lib: 132 passed - github wasm-src cargo test: 58 passed - cargo test -p ironclaw_host_runtime --test github_wasm_runtime_contract --features test-support --all-targets: 48 passed - clippy clean on ironclaw_loop_contracts and github wasm-src
Three CI gates regressed when origin/main was merged into this PR:
1. Reborn PR test planner rejected .claude/skills/promote-run-regression/
SKILL.md as an unclassified path. .claude/ holds agent guidance, not
compiled test surface; add it to IGNORED_PREFIXES.
2. The planner also rejected crates/extensions/packages/<pkg>/{prompts,
schemas, manifest.toml, wasm-src/, wasm/} paths as unmapped crate
paths. WS2 colocated first-party extension packages as siblings of
the support crate; they are not workspace members, so the
workspace-package lookup cannot map them. Route them to
ironclaw_extension_support (the freshness-gate anchor; buckets into
wasm-sandbox) so prompt/schema/guest-source changes run the right
bucket instead of failing fast.
3. check-wasm-artifact-freshness failed: the PR rebuilt the github
wasm from updated wasm-src but did not re-record the source digest.
Rebuild + re-record so the committed artifact matches the recorded
digest. Also drop the accidentally-tracked guest Cargo.lock (guests
resolve fresh; the freshness gate excludes Cargo.lock as non-source)
and gitignore all guest wasm-src lockfiles + target dirs.
Tests:
- python3.11 -m unittest scripts/ci/test_reborn_pr_test_plan.py: 34 OK
(added coverage for .claude/ ignore + extension-package routing)
- python3.11 scripts/ci/check-wasm-artifact-freshness.py: OK, 6 packages
- scripts/ci/test-check-wasm-artifact-freshness.sh: 19 passed, 0 failed
- cargo test -p ironclaw_loop_contracts --lib: 132 passed
- github wasm-src cargo test: 58 passed
github-actions
Bot
added
size: M
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/test_reborn_pr_test_plan.py`:
- Around line 61-67: Update the canonical_packages selection in the helper
method calling planner.build_plan so only a None value falls back to
self.canonical; preserve an explicitly provided empty list by using an explicit
None check instead of truthiness.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3845f5b0-ce68-4627-bcaa-943fee44231a
📒 Files selected for processing (4)
.gitignorescripts/ci/reborn_pr_test_plan.pyscripts/ci/test_reborn_pr_test_plan.pyscripts/ci/wasm-src-digests.toml
| canonical_packages: list[str] | None = None, | ||
| ) -> dict: | ||
| return planner.build_plan( | ||
| event=event, | ||
| changed_paths=paths, | ||
| metadata=metadata(), | ||
| canonical_packages=self.canonical, | ||
| canonical_packages=canonical_packages or self.canonical, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve an explicit empty canonical package list.
Line [67] uses truthiness, so canonical_packages=[] silently selects self.canonical. This prevents the helper from testing an intentionally empty canonical set and can hide the canonical-set failure path. Use an explicit None check.
Proposed fix
- canonical_packages=canonical_packages or self.canonical,
+ canonical_packages=(
+ self.canonical
+ if canonical_packages is None
+ else canonical_packages
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canonical_packages: list[str] | None = None, | |
| ) -> dict: | |
| return planner.build_plan( | |
| event=event, | |
| changed_paths=paths, | |
| metadata=metadata(), | |
| canonical_packages=self.canonical, | |
| canonical_packages=canonical_packages or self.canonical, | |
| canonical_packages: list[str] | None = None, | |
| ) -> dict: | |
| return planner.build_plan( | |
| event=event, | |
| changed_paths=paths, | |
| metadata=metadata(), | |
| canonical_packages=( | |
| self.canonical | |
| if canonical_packages is None | |
| else canonical_packages | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ci/test_reborn_pr_test_plan.py` around lines 61 - 67, Update the
canonical_packages selection in the helper method calling planner.build_plan so
only a None value falls back to self.canonical; preserve an explicitly provided
empty list by using an explicit None check instead of truthiness.
.gitignore is a repository hygiene file: it changes which untracked files a checkout sees, not which workspace crates or tests run. The planner's unclassified-path guard rejected it, failing 'Detect Reborn test scope' on this PR (which adds a gitignore rule for guest wasm-src Cargo.lock/target). Add .gitignore, .dockerignore, .gitattributes to IGNORED_ROOT_PATHS with a reason line, and cover them in the contract tests. python3.11 -m unittest scripts/ci/test_reborn_pr_test_plan.py: 35 OK
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/reborn_pr_test_plan.py (1)
408-413: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
ironclaw_extension_supportto the production canonical package set.
scripts/ci/reborn_pr_test_plan.pyaddsironclaw_extension_supportfor changedcrates/extensions/packages/assets, butscripts/ci/discover-reborn-package-crates.shonly collects Cargo workspace crates fromcargo metadata. That keepschanged_packagesoutsidecanonical_setin CI’s workflow path, which then raiseschanged packages are outside the canonical Reborn package set. Addironclaw_extension_supportto the canonical crate set, plus a CI test exercise without the test’s manually suppliedcanonical_packages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/reborn_pr_test_plan.py` around lines 408 - 413, Add ironclaw_extension_support to the canonical package/crate set used by scripts/ci/discover-reborn-package-crates.sh so it matches the production package added by the reborn_pr_test_plan.py extension-package path. Update the relevant CI test to exercise canonical discovery without manually supplying canonical_packages, ensuring the changed package is included and no outside-canonical-set error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/ci/reborn_pr_test_plan.py`:
- Around line 408-413: Add ironclaw_extension_support to the canonical
package/crate set used by scripts/ci/discover-reborn-package-crates.sh so it
matches the production package added by the reborn_pr_test_plan.py
extension-package path. Update the relevant CI test to exercise canonical
discovery without manually supplying canonical_packages, ensuring the changed
package is included and no outside-canonical-set error occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6e22ff9-a017-43f0-bd46-8f416dc6ebcd
📒 Files selected for processing (2)
scripts/ci/reborn_pr_test_plan.pyscripts/ci/test_reborn_pr_test_plan.py
builtin.result_read built its model-visible observation with
preview: Some(content) unconditionally and relied on downstream
ToolResultReferenceEnvelope::new_best_effort_model_observation repair
(strip_unsafe_result_reference_preview) to drop a credential-bearing
preview later. CI showed a path where the model replay still contained
"preview", so the suppression leaked: the standalone runtime test
standalone_runtime_result_read_suppressed_preview_keeps_durable_continuation_ref
failed in the affected-2 bucket (gateway asserted
!contains("\"preview\""), then the panicked gateway turned the
turn into scheduler_executor_panic).
Construct the preview through ModelResultPreview::new(content).ok() at
the source — the same credential/content gate the initial first-look
preview uses (resolution::result_preview_parts). A chunk that fails the
gate drops only the inline preview; result_ref, total_bytes, next_offset
continuation metadata survives so the model can page past the
suppressed chunk. No longer dependent on best-effort downstream repair.
Tests:
- result_read_observation_drops_preview_for_credential_chunk_but_keeps_continuation
- result_read_observation_keeps_preview_for_ordinary_document_chunk
- cargo test -p ironclaw_loop_host --lib result_read: 3 passed
- cargo test -p ironclaw_reborn_composition --features test-support --lib standalone_runtime_result_read_suppressed_preview_keeps_durable_continuation_ref: ok
- clippy -p ironclaw_loop_host clean
…es.md Code review (local-patterns) found the result-envelope/pagination paragraph appeared twice consecutively after this branch's edit — an appended copy left the original in place. Sibling search_issues_pull_requests.md states the equivalent sentence once. Keep the single-occurrence pattern so future intent edits aren't masked.
Resolve conflicts in scripts/ci/reborn_pr_test_plan.py and test_reborn_pr_test_plan.py. Main landed its own .claude/ classification plus IGNORED_GUIDANCE_PATHS, QA_HARNESS_PREFIXES, INTEGRATION_SUPPORT_OWNERS, and an expanded PR_STATIC_CONTROL_PATHS. Re-apply this branch's two additions on top of main's planner: - EXTENSION_PACKAGES_PREFIX routing (crates/extensions/packages/<pkg>/ prompts/schemas/manifest/wasm-src/wasm -> ironclaw_extension_support; not workspace members, so the workspace-package lookup cannot map them) - IGNORED_ROOT_PATHS (.gitignore/.dockerignore/.gitattributes hygiene files own no Rust/E2E test surface) Re-apply the paired contract tests (repository hygiene ignored; extension package asset routes to support owner). Main's own .claude/ tests cover the third path this branch added earlier. python3.11 -m unittest scripts/ci/test_reborn_pr_test_plan.py: 45 OK python3.11 scripts/ci/check-wasm-artifact-freshness.py: OK, 6 packages PR plan builds clean against the merged tree.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/runtime/tests/core.rs`:
- Around line 1035-1071: Strengthen the suppressed replay assertion in the
suppress_result_read branch of the test harness by verifying that
tool_result.content does not contain the sensitive chunk marker "secret " before
parsing its JSON metadata. Keep the existing preview-field and continuation
metadata assertions unchanged, and ensure the test exercises the real caller
path already covered by this branch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 781ce11b-7efb-453d-a745-d1872df3ce5e
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/runtime/tests/core.rs
| if self.suppress_result_read_preview { | ||
| assert!( | ||
| !tool_result.content.contains("\"preview\""), | ||
| "the rejected chunk preview must remain suppressed" | ||
| ); | ||
| let observation: serde_json::Value = | ||
| serde_json::from_str(&tool_result.content).expect("result_read observation"); | ||
| let detail = &observation["model_observation"]["detail"]; | ||
| let source_result_ref = self | ||
| .source_result_ref | ||
| .lock() | ||
| .expect("source result ref lock poisoned") | ||
| .clone() | ||
| .expect("source result ref captured"); | ||
| assert_eq!( | ||
| detail["result_ref"].as_str(), | ||
| Some(source_result_ref.as_str()), | ||
| "suppressed preview replay must retain the durable source result reference" | ||
| ); | ||
| assert_ne!( | ||
| detail["result_ref"], observation["result_ref"], | ||
| "the inline result_read invocation ref must never become continuation authority" | ||
| ); | ||
| assert!( | ||
| detail["total_bytes"] | ||
| .as_u64() | ||
| .is_some_and(|total_bytes| total_bytes > 2048), | ||
| "suppressed preview replay must retain total bytes: {}", | ||
| tool_result.content | ||
| ); | ||
| assert_eq!( | ||
| detail["next_offset"].as_u64(), | ||
| Some(2048), | ||
| "suppressed preview replay must retain the next continuation offset" | ||
| ); | ||
| return Ok(HostManagedModelResponse::assistant_reply("tool ok")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the sensitive payload is absent from the replay.
Lines 1035-1039 only assert that JSON has no "preview" field. A regression could place the rejected secret ... chunk in another model-visible field and still pass. Assert that tool_result.content does not contain "secret " before parsing the continuation metadata.
As per coding guidelines, “Every new feature and bug fix must begin with a test that demonstrates the intended behavior.” As per path instructions, “Test through the caller” requires the real caller path to prove the side-effect gate.
Proposed test assertion
if self.suppress_result_read_preview {
assert!(
!tool_result.content.contains("\"preview\""),
"the rejected chunk preview must remain suppressed"
);
+ assert!(
+ !tool_result.content.contains("secret "),
+ "the model-visible replay must not expose the rejected sensitive chunk"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.suppress_result_read_preview { | |
| assert!( | |
| !tool_result.content.contains("\"preview\""), | |
| "the rejected chunk preview must remain suppressed" | |
| ); | |
| let observation: serde_json::Value = | |
| serde_json::from_str(&tool_result.content).expect("result_read observation"); | |
| let detail = &observation["model_observation"]["detail"]; | |
| let source_result_ref = self | |
| .source_result_ref | |
| .lock() | |
| .expect("source result ref lock poisoned") | |
| .clone() | |
| .expect("source result ref captured"); | |
| assert_eq!( | |
| detail["result_ref"].as_str(), | |
| Some(source_result_ref.as_str()), | |
| "suppressed preview replay must retain the durable source result reference" | |
| ); | |
| assert_ne!( | |
| detail["result_ref"], observation["result_ref"], | |
| "the inline result_read invocation ref must never become continuation authority" | |
| ); | |
| assert!( | |
| detail["total_bytes"] | |
| .as_u64() | |
| .is_some_and(|total_bytes| total_bytes > 2048), | |
| "suppressed preview replay must retain total bytes: {}", | |
| tool_result.content | |
| ); | |
| assert_eq!( | |
| detail["next_offset"].as_u64(), | |
| Some(2048), | |
| "suppressed preview replay must retain the next continuation offset" | |
| ); | |
| return Ok(HostManagedModelResponse::assistant_reply("tool ok")); | |
| } | |
| if self.suppress_result_read_preview { | |
| assert!( | |
| !tool_result.content.contains("\"preview\""), | |
| "the rejected chunk preview must remain suppressed" | |
| ); | |
| assert!( | |
| !tool_result.content.contains("secret "), | |
| "the model-visible replay must not expose the rejected sensitive chunk" | |
| ); | |
| let observation: serde_json::Value = | |
| serde_json::from_str(&tool_result.content).expect("result_read observation"); | |
| let detail = &observation["model_observation"]["detail"]; | |
| let source_result_ref = self | |
| .source_result_ref | |
| .lock() | |
| .expect("source result ref lock poisoned") | |
| .clone() | |
| .expect("source result ref captured"); | |
| assert_eq!( | |
| detail["result_ref"].as_str(), | |
| Some(source_result_ref.as_str()), | |
| "suppressed preview replay must retain the durable source result reference" | |
| ); | |
| assert_ne!( | |
| detail["result_ref"], observation["result_ref"], | |
| "the inline result_read invocation ref must never become continuation authority" | |
| ); | |
| assert!( | |
| detail["total_bytes"] | |
| .as_u64() | |
| .is_some_and(|total_bytes| total_bytes > 2048), | |
| "suppressed preview replay must retain total bytes: {}", | |
| tool_result.content | |
| ); | |
| assert_eq!( | |
| detail["next_offset"].as_u64(), | |
| Some(2048), | |
| "suppressed preview replay must retain the next continuation offset" | |
| ); | |
| return Ok(HostManagedModelResponse::assistant_reply("tool ok")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/src/runtime/tests/core.rs` around lines
1035 - 1071, Strengthen the suppressed replay assertion in the
suppress_result_read branch of the test harness by verifying that
tool_result.content does not contain the sensitive chunk marker "secret " before
parsing its JSON metadata. Keep the existing preview-field and continuation
metadata assertions unchanged, and ensure the test exercises the real caller
path already covered by this branch.
Sources: Coding guidelines, Path instructions
|
Closing for now. Railway testing showed that the live path still resolves the authenticated user and fans out into per-PR detail calls, so the PR’s one-call behavior is not achieved. The current regression fixture validates a scripted trajectory rather than live model selection. This optimization is nice to have, and a proper fix would require broader changes to the model-visible GitHub contract and realistic canary coverage. We can revisit it separately if tool-call churn becomes a priority. |
This branch was successfully deployed
2 participants
Summary
@medirectly without an unnecessary identity lookup.result_readcontinuation reference when inline preview text is suppressed.Change Type
Linked Issue
Related #6524; Related #5838
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— not run; the focused GitHub WASM crate clippy check passed.cargo build— covered by the focused test builds below; a standalone full-workspace build was not run.cargo test --features integrationif database-backed or integration behavior changed — not applicable; no database behavior changed and the package-qualified recorded integration test passed.result_read), then verified the minimized fixture enforces a one-call authored route without overfitting other self relationships.review-prorpr-shepherd --fixwas run before requesting review — PR remains draft pending normal CI/review.Test Strategy
User behavior:
A user explicitly asking for open
nearai/ironclawPRs they authored receives a prioritized answer after oneauthor: "@me"search. Large provider responses remain bounded, and suppressed preview text cannot destroy the durable continuation authority needed for subsequent reads.Risk areas:
Tests added or updated:
github_open_pr_priority.json, minimized to an explicit authored-by-me request and one self-scoped search rather than copying its ambiguous 65-call trajectory.What the tests prove:
search_issues_pull_requestscall withauthor: "@me", while the prompts preserve distinct authored, assigned, involved, and review-requested relationships.Commands run:
Security Impact
No permission, secret, network-policy, filesystem, sandbox, or approval behavior changes. GitHub credentials remain host-mediated. Provider output still crosses the existing model-visible observation/redaction boundary; rejected credential-like preview text remains suppressed.
Reborn Trust-Boundary Checklist
git diff origin/main...HEAD -- '*error*' '*status*'.serde(default)fields fail closed or have migration tests: not applicable; no persisted schema/default was added.Database Impact
None.
Blast Radius
The first-party GitHub extension’s list/search response shape is intentionally narrower. Consumers needing omitted provider fields must use the existing detail tools. Reborn result observations can now carry continuation metadata without inline preview text; the referenced bytes and persistence semantics are unchanged.
Rollback Plan
Revert the branch commits. This restores the previous GitHub WASM asset/manifest and the prior observation-collapse behavior without a data migration. If only provider compatibility regresses, revert the compaction commit independently while retaining the continuation and regression-test commits.
Review Follow-Through
Please focus reviewer judgment on compact response field sufficiency and the metadata-only continuation observation. The PR is draft so normal CI and maintainer review can run before readiness.
Review track: C (runtime observation behavior and first-party provider execution)