Use QA sheet prompts in Reborn live QA - #5406
Conversation
|
/canary all |
github-actions
Bot
added
size: L
📝 WalkthroughSummary by CodeRabbit
WalkthroughLive QA now reads sheet prompts, allows optional marker checks, routes Slack DM acceptance through signed-event polling, shards live-canary execution, and updates Slack payload grouping. Drive/Sheets lookup text now centers name/title resolution, and memory search is limited to internal persistent memory. ChangesLive QA harness and workflow routing
Reborn Slack payload formatting
Google Drive and Sheets Discovery Metadata
Memory Search Scope Boundary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
|
Started Reborn WebUI v2 live canary for |
![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 refactors the live QA script to centralize test prompts into a new QA_SHEET_PROMPTS dictionary, replacing various inline hardcoded strings. It also updates test cases to handle optional markers (marker: str | None) and adjusts database trigger checks. Feedback on the changes points out that the prompt for qa_3c_endpoint_status_slack_routine contains a literal [endpoint URL] placeholder that is not replaced with a real URL, which could cause the automated test to fail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async def case_qa_3c_endpoint_status_slack_routine(ctx: LiveQaContext) -> ProbeResult: | ||
| marker = "REBORN_QA_3C_ENDPOINT_STATUS_ROUTINE_DONE" | ||
| routine_name = "reborn-qa-3c-endpoint-status-slack" | ||
| return await _routine_creation_case( | ||
| ctx, | ||
| case_name="qa_3c_endpoint_status_slack_routine", | ||
| routine_name=routine_name, | ||
| marker=marker, | ||
| marker=None, | ||
| required_text=["routine"], | ||
| prompt=( | ||
| f"QA case 3C: create a routine named {routine_name}. Every 5 minutes, " | ||
| "ping https://cloud-api.near.ai, check whether it returns HTTP 200, " | ||
| "and send the result in a Slack DM. Create the routine now; do not run " | ||
| "the check immediately. In the final answer include the exact marker " | ||
| f"{marker} and include the text routine." | ||
| ), | ||
| prompt=_qa_sheet_prompt("qa_3c_endpoint_status_slack_routine"), | ||
| ) |
There was a problem hiding this comment.
The prompt retrieved from _qa_sheet_prompt("qa_3c_endpoint_status_slack_routine") contains the literal placeholder [endpoint URL]. Since this is not replaced with a real URL (such as https://cloud-api.near.ai), the agent will receive a prompt with [endpoint URL] and will likely fail to create a valid routine or ask for clarification, causing the automated test to fail or timeout. Please replace [endpoint URL] with a valid URL before passing the prompt to _routine_creation_case.
async def case_qa_3c_endpoint_status_slack_routine(ctx: LiveQaContext) -> ProbeResult:
routine_name = "reborn-qa-3c-endpoint-status-slack"
prompt = _qa_sheet_prompt("qa_3c_endpoint_status_slack_routine").replace(
"[endpoint URL]", "https://cloud-api.near.ai"
)
return await _routine_creation_case(
ctx,
case_name="qa_3c_endpoint_status_slack_routine",
routine_name=routine_name,
marker=None,
required_text=["routine"],
prompt=prompt,
)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py`:
- Line 126: The qa_3c_endpoint_status_slack_routine prompt still contains an
unresolved “[endpoint URL]” placeholder, so the generated routine will have a
bogus ping target. Update the string in _qa_sheet_prompt / run_live_qa.py to use
a concrete endpoint value or an explicit template variable that is filled before
use, and make sure the routine creation path cannot pass the placeholder through
unchanged.
🪄 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: 43320b8c-d9e5-4c9d-b110-bbd88b0594db
📒 Files selected for processing (2)
scripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
|
/canary all |
|
Started Reborn WebUI v2 live canary for |
|
🚅 Deployed to the ironclaw-pr-5406 environment in ironclaw-ci-preview
|
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 (3)
scripts/reborn_webui_v2_live_qa/run_live_qa.py (3)
1156-1164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the live status code, not just the word
status.Line 1157 computes
live_status, but Line 1163 never requires it. A failure reply like “I can’t check the status” can pass.Proposed fix
- required_text=["status"], + required_text=["status", str(live_status)],🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 1156 - 1164, The live QA check in _live_chat_case is only asserting the presence of the word “status” and not the actual live status code computed by _live_http_status. Update the qa_3b_endpoint_status_live_chat call to require the expected_status_code value (or equivalent exact status text) in required_text or the assertion logic, so the response must match the live result rather than a generic status mention.
2533-2549: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not validate markerless routines with an unscoped trigger count.
Line 2533 sets
count_name=Nonewhenmarkeris absent, so Lines 2547-2549 can pass if any unrelated trigger record appears. Validate a record tied to this request instead: timestamp/session/user prompt/routine id, or keep a deterministic QA marker in the created routine.🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 2533 - 2549, The trigger-count check in the live QA flow is too broad when `marker` is absent because `count_name` becomes None and `_trigger_record_count` can be satisfied by unrelated records. Update the logic around `count_name`, `_trigger_record_count`, and `_live_chat_case` so the post-run validation is scoped to this specific request, using a deterministic routine marker or another unique identifier such as routine id/session/prompt/timestamp. Ensure the success condition compares the same request-bound record before and after, rather than any global trigger record.
846-851: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind markerless waits to the newly submitted prompt.
Line 1005 makes
marker=Noneaccept any assistant text withrequired_text. After the markerless migrations, stale chat history can satisfy the probe before the new answer arrives. Capture the assistant-message count before Line 830 and only inspect messages added after submission.Proposed fix
+ assistant_count_before = await page.locator("[data-testid='msg-assistant']").count() await composer.fill(prompt) await composer.press("Enter") @@ observed["text_excerpt"] = await _wait_for_assistant_reply( page, marker=marker, required_text=required_text, timeout=timeout, + assistant_count_before=assistant_count_before, ) @@ async def _wait_for_assistant_reply( page: object, *, marker: str | None, required_text: list[str], timeout: float, + assistant_count_before: int = 0, ) -> str: @@ - assistant = page.locator("[data-testid='msg-assistant']").last # type: ignore[attr-defined] + assistants = page.locator("[data-testid='msg-assistant']") # type: ignore[attr-defined] @@ - if await assistant.count() > 0: + assistant_count = await assistants.count() + if assistant_count > assistant_count_before: + assistant = assistants.nth(assistant_count - 1)Also applies to: 985-1007
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 846 - 851, The markerless wait in _wait_for_assistant_reply can match stale assistant text from earlier chat history instead of the newly submitted prompt. Capture the assistant-message count before the submission in the surrounding live QA flow, then update _wait_for_assistant_reply so it only inspects assistant messages added after that baseline when marker is None. Make the change in the wait/probe logic used by observed["text_excerpt"] and the related markerless path so the returned text is bound to the latest prompt.
🤖 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/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 256-281: Tighten the test for
case_qa_3c_endpoint_status_slack_routine so it also verifies the markerless
contract, not just ENDPOINT_STATUS_URL substitution. Update the
_trigger_record_count stub and assertions to confirm the routine passes
marker=None into _live_chat_case, using the captured kwargs in
fake_live_chat_case to assert the marker field is absent or explicitly None.
This keeps the test anchored to case_qa_3c_endpoint_status_slack_routine,
_live_chat_case, and _trigger_record_count.
---
Outside diff comments:
In `@scripts/reborn_webui_v2_live_qa/run_live_qa.py`:
- Around line 1156-1164: The live QA check in _live_chat_case is only asserting
the presence of the word “status” and not the actual live status code computed
by _live_http_status. Update the qa_3b_endpoint_status_live_chat call to require
the expected_status_code value (or equivalent exact status text) in
required_text or the assertion logic, so the response must match the live result
rather than a generic status mention.
- Around line 2533-2549: The trigger-count check in the live QA flow is too
broad when `marker` is absent because `count_name` becomes None and
`_trigger_record_count` can be satisfied by unrelated records. Update the logic
around `count_name`, `_trigger_record_count`, and `_live_chat_case` so the
post-run validation is scoped to this specific request, using a deterministic
routine marker or another unique identifier such as routine
id/session/prompt/timestamp. Ensure the success condition compares the same
request-bound record before and after, rather than any global trigger record.
- Around line 846-851: The markerless wait in _wait_for_assistant_reply can
match stale assistant text from earlier chat history instead of the newly
submitted prompt. Capture the assistant-message count before the submission in
the surrounding live QA flow, then update _wait_for_assistant_reply so it only
inspects assistant messages added after that baseline when marker is None. Make
the change in the wait/probe logic used by observed["text_excerpt"] and the
related markerless path so the returned text is bound to the latest prompt.
🪄 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: 786fdae1-b093-43c7-89cd-3ec6f6305456
📒 Files selected for processing (2)
scripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
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)
scripts/live-canary/test_notify_slack.py (1)
336-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for the multi-block QA path.
This test still only proves the single-section case. The new split logic in
scripts/live-canary/notify_slack.pycan break overflow rendering without failing CI. Please force one QA group past the Slack limit and assert that every emitted section still carries the same group label and all failure lines survive.🤖 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/live-canary/test_notify_slack.py` around lines 336 - 360, The Slack notification test only covers a single QA block, so it misses regressions in overflow splitting. Extend the existing assertions in test_notify_slack.py around the QA 2 section to force one QA group past the Slack limit and verify the split output from notify_slack.py still repeats the same group label in every emitted section. Also assert all expected failure lines remain present across the split sections, using the existing qa_sections/qa_text checks as the anchor.
🤖 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/live-canary/notify_slack.py`:
- Around line 643-667: The Slack section chunking logic in notify_slack.py drops
the QA group header on overflow blocks, making continuation sections ambiguous.
Update the block-building flow in the loop that appends to blocks so each new
section created after a split includes the same `QA {group}` label/header before
the continuation content. Keep the fix localized to the chunking code that uses
`current`, `candidate`, and `_trim_slack_block_text`, and ensure the repeated
header is preserved whenever a long group spans multiple Slack sections.
---
Outside diff comments:
In `@scripts/live-canary/test_notify_slack.py`:
- Around line 336-360: The Slack notification test only covers a single QA
block, so it misses regressions in overflow splitting. Extend the existing
assertions in test_notify_slack.py around the QA 2 section to force one QA group
past the Slack limit and verify the split output from notify_slack.py still
repeats the same group label in every emitted section. Also assert all expected
failure lines remain present across the split sections, using the existing
qa_sections/qa_text checks as the anchor.
🪄 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: df58f72d-1908-4aaa-8e25-a45ed08a6189
📒 Files selected for processing (2)
scripts/live-canary/notify_slack.pyscripts/live-canary/test_notify_slack.py
github-actions
Bot
added
scope: ci
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/reborn_webui_v2_live_qa/run_live_qa.py (1)
126-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not pass the endpoint placeholder into the routine prompt.
_qa_sheet_prompt("qa_3c_endpoint_status_slack_routine")still returns[endpoint URL]verbatim, so the routine can be created with a bogus ping target instead ofENDPOINT_STATUS_URLor a filled template value.🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` at line 126, The qa_3c_endpoint_status_slack_routine prompt still contains the literal endpoint placeholder, so it can produce a bogus ping target. Update the prompt source in run_live_qa.py and the _qa_sheet_prompt("qa_3c_endpoint_status_slack_routine") flow so it uses ENDPOINT_STATUS_URL or a substituted runtime value instead of passing "[endpoint URL]" through verbatim. Verify the routine text is fully templated before the Slack DM instruction is generated.
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py`:
- Around line 3003-3005: The Slack payload built in run_live_qa.py is using the
QA sheet prompt instead of a real bug report, which weakens the trigger canary.
Update the logic around the slack_user_id/text/event_id setup so the observed
field can still keep _qa_sheet_prompt(case_name), but the actual text sent to
Slack is a deterministic concrete bug fixture like “bug: reborn QA bug logger
smoke {suffix}”. Keep the change localized to the live QA event construction so
the sent message no longer mirrors the operator instruction.
- Around line 3214-3231: The custom prompt flow in run_live_qa.py is reusing
stale capability history by accepting any prior "completed" status for the
capability IDs. Update the loop around _approve_visible_tool_gate and
_capability_run_statuses so it only accepts capability evidence generated after
this prompt has been submitted and the assistant has replied in the current
chat, rather than trusting pre-existing completed entries. Use the existing
capability_ids, observed, and ctx.reborn_home helpers to scope the status check
to the current run before returning success.
---
Duplicate comments:
In `@scripts/reborn_webui_v2_live_qa/run_live_qa.py`:
- Line 126: The qa_3c_endpoint_status_slack_routine prompt still contains the
literal endpoint placeholder, so it can produce a bogus ping target. Update the
prompt source in run_live_qa.py and the
_qa_sheet_prompt("qa_3c_endpoint_status_slack_routine") flow so it uses
ENDPOINT_STATUS_URL or a substituted runtime value instead of passing "[endpoint
URL]" through verbatim. Verify the routine text is fully templated before the
Slack DM instruction is generated.
🪄 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: 099f412f-c816-40f5-9ce3-294ef93618b5
📒 Files selected for processing (5)
.github/workflows/live-canary.ymlscripts/live-canary/ACCOUNTS.mdscripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/slack_helpers.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
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 (2)
scripts/reborn_webui_v2_live_qa/run_live_qa.py (2)
2596-2603: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't send the clarification turn solely on missing trigger growth.
This retry fires whenever the count has not increased yet, even if the first assistant reply already completed the request and the trigger write is just lagging. In live QA that can send an extra instruction into a successful conversation and create or mutate the routine twice.
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 2596 - 2603, The retry logic in _live_chat_case is too eager: it sends clarification whenever after_count does not exceed before_count, even if the first assistant response already satisfied the request. Update the condition around clarification_reply so it only retries when the conversation actually needs it, not just because trigger growth is missing or delayed; keep the check localized to the live QA flow where after_count, before_count, and clarification_reply are evaluated.
2580-2581: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep trigger counting scoped to
routine_namefor markerless routines.When
markerisNone,count_namebecomesNone, so both the success gate and the clarification retry use the global trigger count. Any unrelated trigger write in this case home can make the routine look created, or suppress the follow-up, even whenroutine_nameitself never produced a record.Proposed fix
- count_name = routine_name if marker else None + count_name = routine_name🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 2580 - 2581, The trigger counting in the routine creation flow is too broad for markerless routines because `count_name` becomes `None`, causing `_trigger_record_count` and the success/clarification checks to use the global count instead of the specific routine. Update the logic around `routine_name`, `marker`, `count_name`, and `_trigger_record_count` so markerless routines still count only records tied to `routine_name`, and keep the success gate plus clarification retry scoped to that routine rather than any unrelated trigger writes.
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py`:
- Around line 2596-2603: The retry logic in _live_chat_case is too eager: it
sends clarification whenever after_count does not exceed before_count, even if
the first assistant response already satisfied the request. Update the condition
around clarification_reply so it only retries when the conversation actually
needs it, not just because trigger growth is missing or delayed; keep the check
localized to the live QA flow where after_count, before_count, and
clarification_reply are evaluated.
- Around line 2580-2581: The trigger counting in the routine creation flow is
too broad for markerless routines because `count_name` becomes `None`, causing
`_trigger_record_count` and the success/clarification checks to use the global
count instead of the specific routine. Update the logic around `routine_name`,
`marker`, `count_name`, and `_trigger_record_count` so markerless routines still
count only records tied to `routine_name`, and keep the success gate plus
clarification retry scoped to that routine rather than any unrelated trigger
writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c355298c-b6bf-455e-bd4a-20e3035e869d
📒 Files selected for processing (4)
scripts/live-canary/ACCOUNTS.mdscripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/slack_helpers.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/live-canary.yml:
- Around line 586-589: The live-canary workflow is checking out and later
executing code from inputs.target_ref, which can let PR-controlled code run with
live secrets. Update the checkout in the live-secret path to use only a trusted
ref, and ensure the secret-bearing steps around the harness execution do not
consume code from the untrusted target ref. If target code must still be tested,
split it into a separate no-secret checkout or path before the privileged steps
that run after secrets are materialized.
- Around line 573-577: The shard-skip branch in the live-canary workflow
currently treats an empty `selected` set as a harmless skip, which lets invalid
`inputs.cases` values pass green across all matrix shards. Add a pre-matrix or
global validation step in the workflow to compare the requested case names
against the full union of known shard cases before the shard-selection logic
runs, and fail the job when any requested name is unknown. Keep the existing
shard skip path only for valid requests that simply do not belong to the current
shard, and use the `selected`/`skip_shard` flow to distinguish that case from
invalid input.
🪄 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: aa869006-e69a-4474-a61a-e3e9b9a98629
📒 Files selected for processing (2)
.github/workflows/live-canary.ymlscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
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/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 306-318: Tighten the test doubles to match the real helper
contracts instead of accepting arbitrary kwargs. Update the fake helpers used in
fake_live_chat_with_extensions_case and the related routine creation cases so
they explicitly assert the expected arguments, including access_token forwarding
from case_qa_7c_slack_bug_logger_routine and marker=None plus required_text in
_routine_creation_case. This should make the tests fail if those required
parameters stop being passed, rather than silently swallowing regressions
through **kwargs.
🪄 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: f98f45d8-61f0-4197-a2df-b0ac617ac840
📒 Files selected for processing (3)
scripts/reborn_webui_v2_live_qa/google_api_helpers.pyscripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/live-canary/notify_slack.py (1)
697-729: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the fallback text on appended QA blocks, not
reborn_qa_cases.
renders_reborn_qa_groups = bool(r.reborn_qa_cases)suppresses the lane-level failure/reason/tools/notable text before you know whether any grouped QA blocks will fit underSLACK_MAX_BLOCKS. If the remaining capacity is 0, this lane renders only the summary header and drops all failure context. Base the guard on the sliced_format_reborn_qa_groups(...)result you actually append.Possible fix
- renders_reborn_qa_groups = bool(r.reborn_qa_cases) + qa_group_blocks = _format_reborn_qa_groups(r.reborn_qa_cases) if r.reborn_qa_cases else [] + remaining_qa_group_blocks = max(0, SLACK_MAX_BLOCKS - (len(blocks) + 1)) + rendered_qa_group_blocks = qa_group_blocks[:remaining_qa_group_blocks] + renders_reborn_qa_groups = bool(rendered_qa_group_blocks) @@ - if renders_reborn_qa_groups: - ... + if renders_reborn_qa_groups: + blocks.extend(rendered_qa_group_blocks)🤖 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/live-canary/notify_slack.py` around lines 697 - 729, The lane summary in the Slack formatter is suppressing failure/reason/tools/notable text based on reborn_qa_cases instead of what actually gets appended, which can drop context when grouped QA blocks do not fit. Update the logic in the Slack block builder around the header_line/lines assembly to base the fallback-text guard on the sliced _format_reborn_qa_groups(...) result you append, and only suppress the lane-level details when those QA blocks are truly rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/live-canary.yml:
- Around line 551-558: ALL_SHARD_CASES is manually duplicated and can drift from
the matrix definition, so update the live-canary workflow to derive this value
from the same shard case source used by the matrix instead of hardcoding a
second list. Adjust the workflow logic around ALL_SHARD_CASES so it is generated
or validated from the matrix cases, keeping the shard names in sync
automatically.
In `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 534-535: The test stub in fake_capability_run_statuses should
verify the capability IDs it receives instead of ignoring them, so the case
stays pinned to QA_7A_CHAT_CONNECT_CAPABILITY_IDS. Update the stub to assert the
_capability_ids argument matches the expected set used by
case_qa_7a_slack_product_channel_connect(), while keeping the existing
status_sequence behavior for polling.
- Around line 624-633: The ordering evidence in captured_routine is using
mutable references for extensions and extra_details, so later mutations can
invalidate the test without failing. Update the snapshot logic in
test_run_live_qa.py around captured_routine to deep-copy those values before
storing them, and add the stdlib copy import near the other imports. Keep the
fix localized to the captured_routine update and the related assertions that
rely on it.
---
Outside diff comments:
In `@scripts/live-canary/notify_slack.py`:
- Around line 697-729: The lane summary in the Slack formatter is suppressing
failure/reason/tools/notable text based on reborn_qa_cases instead of what
actually gets appended, which can drop context when grouped QA blocks do not
fit. Update the logic in the Slack block builder around the header_line/lines
assembly to base the fallback-text guard on the sliced
_format_reborn_qa_groups(...) result you append, and only suppress the
lane-level details when those QA blocks are truly rendered.
🪄 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: a513be1b-9f09-4bfc-9766-180a3e1cff23
📒 Files selected for processing (5)
.github/workflows/live-canary.ymlscripts/live-canary/notify_slack.pyscripts/live-canary/test_notify_slack.pyscripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
| ALL_SHARD_CASES: >- | ||
| qa_2a_gmail_connect,qa_2b_calendar_connect,qa_2c_drive_connect,qa_2d_calendar_prep_live_chat,qa_2e_calendar_prep_email_routine,qa_2f_calendar_prep_email_delivery, | ||
| qa_3a_slack_connect,qa_3b_endpoint_status_live_chat,qa_3c_endpoint_status_slack_routine,qa_3d_endpoint_status_slack_delivery, | ||
| qa_4a_gmail_connect,qa_4b_github_connect,qa_4c_github_release_live_chat,qa_4d_github_release_slack_routine,qa_4e_github_release_email_delivery, | ||
| qa_5a_slack_connect,qa_5b_drive_connect,qa_5c_strategy_doc_knowledge_base,qa_5d_slack_strategy_doc_answer, | ||
| qa_6a_gmail_connect,qa_6b_sheets_connect,qa_6c_gmail_to_sheet_live_chat,qa_6d_gmail_to_sheet_routine,qa_6e_gmail_to_sheet_delivery, | ||
| qa_7a_slack_product_channel_connect,qa_7b_sheets_connect,qa_7c_slack_bug_logger_routine,qa_7d_slack_bug_message_trigger,qa_7e_slack_bug_sheet_delivery, | ||
| qa_8a_slack_connect,qa_8b_hn_keyword_live_chat,qa_8c_hn_keyword_slack_routine,qa_8d_hn_keyword_slack_delivery |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm matrix.cases union == ALL_SHARD_CASES (order-independent set compare)
f=.github/workflows/live-canary.yml
python3 - "$f" <<'PY'
import sys,re,yaml
doc=yaml.safe_load(open(sys.argv[1]))
job=doc['jobs']['reborn-webui-v2-live-qa']
matrix=set()
for inc in job['strategy']['matrix']['include']:
matrix |= {c.strip() for c in inc['cases'].split(',') if c.strip()}
allc=set()
for step in job['steps']:
env=step.get('env',{})
if 'ALL_SHARD_CASES' in env:
allc={c.strip() for c in env['ALL_SHARD_CASES'].split(',') if c.strip()}
print("only in matrix:", sorted(matrix-allc))
print("only in ALL_SHARD_CASES:", sorted(allc-matrix))
print("in sync:", matrix==allc)
PYRepository: nearai/ironclaw
Length of output: 214
Derive ALL_SHARD_CASES from the matrix cases instead of hand-maintaining a second copy. Drift here can reject valid QA shards or admit invalid ones; a small sync guard or generated list removes that risk.
🤖 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 @.github/workflows/live-canary.yml around lines 551 - 558, ALL_SHARD_CASES is
manually duplicated and can drift from the matrix definition, so update the
live-canary workflow to derive this value from the same shard case source used
by the matrix instead of hardcoding a second list. Adjust the workflow logic
around ALL_SHARD_CASES so it is generated or validated from the matrix cases,
keeping the shard names in sync automatically.
Source: Path instructions
| def fake_capability_run_statuses(_reborn_home, _capability_ids): | ||
| return status_sequence.pop(0) if status_sequence else fresh |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the queried capability IDs in the stub.
Lines 534-535 ignore _capability_ids, so this test still passes if case_qa_7a_slack_product_channel_connect() starts polling the wrong capability set. That weakens the exact QA_7A_CHAT_CONNECT_CAPABILITY_IDS contract this case is supposed to pin.
Suggested fix
- def fake_capability_run_statuses(_reborn_home, _capability_ids):
+ def fake_capability_run_statuses(_reborn_home, queried_capability_ids):
+ self.assertEqual(queried_capability_ids, capability_ids)
return status_sequence.pop(0) if status_sequence else fresh📝 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.
| def fake_capability_run_statuses(_reborn_home, _capability_ids): | |
| return status_sequence.pop(0) if status_sequence else fresh | |
| def fake_capability_run_statuses(_reborn_home, queried_capability_ids): | |
| self.assertEqual(queried_capability_ids, capability_ids) | |
| return status_sequence.pop(0) if status_sequence else fresh |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 534-534: Missing return type annotation for private function fake_capability_run_statuses
(ANN202)
🤖 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/reborn_webui_v2_live_qa/test_run_live_qa.py` around lines 534 - 535,
The test stub in fake_capability_run_statuses should verify the capability IDs
it receives instead of ignoring them, so the case stays pinned to
QA_7A_CHAT_CONNECT_CAPABILITY_IDS. Update the stub to assert the _capability_ids
argument matches the expected set used by
case_qa_7a_slack_product_channel_connect(), while keeping the existing
status_sequence behavior for polling.
| captured_routine.update( | ||
| { | ||
| "case_name": case_name, | ||
| "prompt": prompt, | ||
| "marker": marker, | ||
| "routine_name": routine_name, | ||
| "required_text": required_text, | ||
| "extensions": extensions, | ||
| "extra_details": extra_details, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Snapshot mutable args before using them as ordering evidence.
Lines 624-633 store extensions and extra_details by reference. If case_qa_7c_slack_bug_logger_routine() mutates either after awaiting _routine_creation_case(), the assertions at Lines 688-691 still pass, so this test no longer proves the sheet fixture existed before the routine call.
Suggested fix
- captured_routine.update(
- {
- "case_name": case_name,
- "prompt": prompt,
- "marker": marker,
- "routine_name": routine_name,
- "required_text": required_text,
- "extensions": extensions,
- "extra_details": extra_details,
- }
- )
+ captured_routine.update(
+ copy.deepcopy(
+ {
+ "case_name": case_name,
+ "prompt": prompt,
+ "marker": marker,
+ "routine_name": routine_name,
+ "required_text": required_text,
+ "extensions": extensions,
+ "extra_details": extra_details,
+ }
+ )
+ )Also add import copy with the stdlib imports.
🤖 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/reborn_webui_v2_live_qa/test_run_live_qa.py` around lines 624 - 633,
The ordering evidence in captured_routine is using mutable references for
extensions and extra_details, so later mutations can invalidate the test without
failing. Update the snapshot logic in test_run_live_qa.py around
captured_routine to deep-copy those values before storing them, and add the
stdlib copy import near the other imports. Keep the fix localized to the
captured_routine update and the related assertions that rely on it.
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_host_runtime/src/first_party_tools/schemas.rs`:
- Around line 49-53: Update the `builtin.memory_search` schema in `schemas.rs`
so its `description` and `properties.query.description` match the current-scope
contract from `memory.rs` and do not imply a broader “internal persistent
memory” surface. Use the same tenant/user/agent/project/mission/thread scope
wording already advertised by the runtime, and add or adjust the relevant
surface test to pin this exact schema text so the model-facing contract stays
consistent.
🪄 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: 97b19587-14a0-41c0-9026-6cc81013ef59
📒 Files selected for processing (3)
crates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
| "description": "Searches only Reborn internal persistent memory. This does not search connected app or extension data.", | ||
| "properties": { | ||
| "query": { | ||
| "type": "string", | ||
| "description": "Preferred natural language search query for persistent memory" | ||
| "description": "Preferred natural language search query for Reborn internal persistent memory" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Schema wording drops the caller-scope boundary.
crates/ironclaw_host_runtime/src/first_party_tools/memory.rs still advertises builtin.memory_search as limited to the current tenant/user/agent/project scope, but this schema now only says “Reborn internal persistent memory.” That weakens the model-facing contract across layers and can imply a broader search surface than the runtime actually exposes. Mirror the current-scope wording in both the schema description and properties.query.description, then pin it in the surface test. As per path instructions, "Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records."
Suggested patch
- "description": "Searches only Reborn internal persistent memory. This does not search connected app or extension data.",
+ "description": "Searches only Reborn internal persistent memory in the current tenant/user/agent/project scope. This does not search connected app or extension data.",
"properties": {
"query": {
"type": "string",
- "description": "Preferred natural language search query for Reborn internal persistent memory"
+ "description": "Preferred natural language search query for Reborn internal persistent memory in the current tenant/user/agent/project scope"
},📝 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.
| "description": "Searches only Reborn internal persistent memory. This does not search connected app or extension data.", | |
| "properties": { | |
| "query": { | |
| "type": "string", | |
| "description": "Preferred natural language search query for persistent memory" | |
| "description": "Preferred natural language search query for Reborn internal persistent memory" | |
| "description": "Searches only Reborn internal persistent memory in the current tenant/user/agent/project scope. This does not search connected app or extension data.", | |
| "properties": { | |
| "query": { | |
| "type": "string", | |
| "description": "Preferred natural language search query for Reborn internal persistent memory in the current tenant/user/agent/project scope" |
🤖 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/src/first_party_tools/schemas.rs` around lines
49 - 53, Update the `builtin.memory_search` schema in `schemas.rs` so its
`description` and `properties.query.description` match the current-scope
contract from `memory.rs` and do not imply a broader “internal persistent
memory” surface. Use the same tenant/user/agent/project/mission/thread scope
wording already advertised by the runtime, and add or adjust the relevant
surface test to pin this exact schema text so the model-facing contract stays
consistent.
Source: Path instructions
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 (3)
scripts/reborn_webui_v2_live_qa/run_live_qa.py (3)
3291-3298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t pin HN success to the raw hostname.
The QA sheet only requires reporting matching Hacker News posts. Requiring
news.ycombinator.comrejects valid replies that say “Hacker News” or summarize titles without the literal host string, so this adds avoidable false negatives to the canary.🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 3291 - 3298, The `case_qa_8b_hn_keyword_live_chat` check is too strict by requiring the raw hostname `news.ycombinator.com`, which causes false negatives for otherwise valid Hacker News responses. Update the `required_text` in `_live_chat_case` for this case to accept the QA sheet’s intended HN wording (for example, “Hacker News” or equivalent post summaries) instead of pinning validation to the literal host string. Keep the change localized to `case_qa_8b_hn_keyword_live_chat` so the canary still verifies the right content without overfitting to a URL.
112-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStop sending
Expected result:text to the agent.
_qa_sheet_prompt()returns the full QA row, and every mapped value here includes anExpected result:line. The live chat/routine helpers submit that verbatim, so the oracle is now in the prompt body and cases can pass by echoing the expected text instead of actually doing the work.Patch sketch
def _qa_sheet_prompt(case_name: str) -> str: try: - return QA_SHEET_PROMPTS[case_name] + entry = QA_SHEET_PROMPTS[case_name] except KeyError as exc: raise AssertionError(f"QA sheet prompt is not hardcoded for {case_name}") from exc + return entry.split("\nExpected result:", 1)[0].strip()🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 112 - 174, The QA prompt mapping in QA_SHEET_PROMPTS is leaking oracle text into the agent input because _qa_sheet_prompt() returns rows that include the “Expected result:” line. Update the prompt construction used by the live chat/routine helpers so they only send the task instructions and strip or exclude the expected-outcome text; keep QA_SHEET_PROMPTS focused on the action text, and make sure _qa_sheet_prompt() no longer returns the expected-result portion for any case_name.
3159-3165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat “Routine/trigger created” as an OR, not an AND.
required_text=["trigger", "bug"]can fail a valid success like “I created the routine to log bug messages,” even after_trigger_record_countproves the trigger was created. Keep the DB check as the creation proof and loosen the reply matcher here.🤖 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/reborn_webui_v2_live_qa/run_live_qa.py` around lines 3159 - 3165, The success check in qa_7c_slack_bug_logger_routine is too strict because it requires both “trigger” and “bug” in the reply, which can reject valid confirmations even when _trigger_record_count already proves creation. Update _routine_creation_case usage here to loosen the required_text matcher so the response can pass with either routine/trigger creation wording or bug logging wording, while keeping the database count check as the source of truth.
🤖 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/reborn_webui_v2_live_qa/run_live_qa.py`:
- Around line 3291-3298: The `case_qa_8b_hn_keyword_live_chat` check is too
strict by requiring the raw hostname `news.ycombinator.com`, which causes false
negatives for otherwise valid Hacker News responses. Update the `required_text`
in `_live_chat_case` for this case to accept the QA sheet’s intended HN wording
(for example, “Hacker News” or equivalent post summaries) instead of pinning
validation to the literal host string. Keep the change localized to
`case_qa_8b_hn_keyword_live_chat` so the canary still verifies the right content
without overfitting to a URL.
- Around line 112-174: The QA prompt mapping in QA_SHEET_PROMPTS is leaking
oracle text into the agent input because _qa_sheet_prompt() returns rows that
include the “Expected result:” line. Update the prompt construction used by the
live chat/routine helpers so they only send the task instructions and strip or
exclude the expected-outcome text; keep QA_SHEET_PROMPTS focused on the action
text, and make sure _qa_sheet_prompt() no longer returns the expected-result
portion for any case_name.
- Around line 3159-3165: The success check in qa_7c_slack_bug_logger_routine is
too strict because it requires both “trigger” and “bug” in the reply, which can
reject valid confirmations even when _trigger_record_count already proves
creation. Update _routine_creation_case usage here to loosen the required_text
matcher so the response can pass with either routine/trigger creation wording or
bug logging wording, while keeping the database count check as the source of
truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a45368b3-47b9-4cb7-9ee9-ac119fa1bc43
📒 Files selected for processing (2)
scripts/reborn_webui_v2_live_qa/run_live_qa.pyscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
This branch was successfully deployed
1 participant
Summary
Known Product Gap Surfaced
bug:, append a row to this Google Sheet” Slack-event trigger workflow. That case should not require an outbound Slack delivery target; Slack is the event source and Google Sheets is the side effect.Verification