Skip to content

test(e2e): count capability coverage per outcome, not per capability - #6664

Merged
serrrfirat merged 7 commits into
mainfrom
firat/capability-outcome-coverage-gate
Jul 24, 2026
Merged

serrrfirat merged 7 commits into
mainfrom
firat/capability-outcome-coverage-gate

Conversation

@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

  • The capability inventory reported 123/123 tested, zero waivers, but tested was satisfiable three ways and one of them proved nothing: _recorded_tool_evidence credits a capability when any harvested trace emits a tool_calls entry with that name. For a write, a recorded model response naming slack__send_message proves the model chose the tool and says nothing about whether the provider committed the effect.
  • Eight external_write capabilities rested on that proxy. Auditing them found six do have provider readback inside _assert_google_provider_outcome / _assert_slack_provider_outcome — but the gate could not see it, so deleting those assertions would have left the inventory green. The other two (google-sheets.write_values, google-sheets.rename_sheet) are issued by qa_7e alongside append_values against one spreadsheet whose single asserted row cannot attribute the mutation.
  • Coverage is now counted per capability × outcome class. Writes may not rest on a tool-call name; reads need a seeded success and an empty case (epic Epic: Hermetic capability and journey testing platform #6524 workstream 5). The read/write split is derived from external_write in each tool's effects in the shipped manifests, so a new tool classifies itself.
  • The six real-readback writes are now declared as journey_evidence naming the exact test and its assertion helper, both verified to exist. The two unproven ones got typed cases with isolating readback. google-sheets.read_values gained the first success/empty pair.
  • Everything not yet meeting the rules is in coverage_backlog with owner/reason/issue/review-condition. Write backlog: 0. Read backlog: 48 — a gap that was previously invisible.

Change Type

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • CI/Infrastructure
  • Security
  • Dependencies

Linked Issue

Related #6524 (workstreams 4, 5, 11). Not stacked on #6659.

Validation

  • cargo fmt --all -- --check — not applicable: no Rust changed
  • cargo clippy ... — not applicable: no Rust changed
  • cargo build -p ironclaw --bin ironclaw (needed to execute the new cases)
  • Relevant tests pass: 135 passed, 0 failed, 0 skipped in 2m52s — test_provider_operation_case_executes_with_provider_readback (100 cases), test_provider_capability_inventory.py, test_journey_coverage.py, against pinned serrrfirat/emulate c85da1e and a real ironclaw serve
  • cargo test --features integration — not applicable: no database-backed behavior changed
  • Manual testing: mutation-checked the gate and the new cases (below)
  • review-pr / pr-shepherd --fix — not run (agent session; command evidence inline)

Test Strategy

User behavior: none changed — tests, fixtures, and docs only. No production crate is touched.

Risk areas:

  • Model behavior
  • Browser
  • Side effect
  • Persistence
  • Security or permissions
  • External provider
  • Cross-component behavior (coverage gating)

Tests added or updated:

  • Unit or contract: test_write_capabilities_are_not_evidenced_by_a_recorded_tool_name, test_read_capabilities_cover_every_required_outcome_class, test_coverage_backlog_entries_are_owned_and_not_stale, test_journey_evidence_names_an_executable_assertion, test_journey_evidence_rejects_a_vanished_symbol
  • Reborn integration: none
  • Recorded fixture: none — deliberately, since crediting a recorded fixture for a write is the bug being fixed
  • Browser E2E: none
  • Backend or runtime: 4 new ProviderOperationCase entries executed through standalone Reborn + Emulate
  • Live canary: none

What the tests prove. The gate is mutation-verified rather than assumed. Each of these turns the suite red, and the baseline restores green:

Mutation Result
Drop a journey_evidence block fails
Point an assertion at a deleted symbol fails
Remove a capability from the read backlog fails
Backlog a capability that already has a case (stale entry) fails
Change write_values' expected cell contents fails — readback is load-bearing

Two of those are pinned permanently as harness self-tests.

The new cases isolate what qa_7e could not: write_values pins the exact Sheet1!A4:C4 range below the seeded rows, so an append cannot satisfy it, and re-asserts the seeded row is untouched; rename_sheet asserts the same sheetId carries the new title, proving a rename rather than a delete-and-recreate.

The empty case asserts a positive contract — the model receives {"range": "Sheet1!A50:C60", "values": []} at status: completed. Its first draft only checked that seeded markers had not leaked and that no error string appeared; dumping the real payload showed that form would pass on a payload proving nothing, so it was rewritten against the actual contract.

Commands run:

cargo build -p ironclaw --bin ironclaw
IRONCLAW_EMULATE_CLI=.emulate/packages/emulate/dist/index.js pytest \
  tests/e2e/scenarios/test_provider_capability_inventory.py \
  tests/e2e/scenarios/test_journey_coverage.py \
  "tests/e2e/scenarios/test_reborn_qa_trace_full_path.py::test_provider_operation_case_executes_with_provider_readback" \
  --timeout=180

Reviewer notes

  • Scope: this makes the true coverage number visible and blocks regression. It does not close the 48-capability read gap — that is enumerated in coverage_backlog, each entry with an owner and a review condition, and is follow-up work.
  • Two silent skips found in passing, not fixed here (out of scope, worth issues): _assert_slack_provider_outcome returns early when a trace has no slack__send_message calls, and _assert_google_provider_outcome returns early when _google_created_resource_call is None. Both make the assertion vanish rather than fail when a trace changes shape.
  • The epic's acceptance criterion "Every production capability is represented by executable hermetic evidence — landed at 123/123 with zero waivers" should probably be restated in Epic: Hermetic capability and journey testing platform #6524: for 23 capabilities that evidence was a name in a JSON file.

🤖 Generated with Claude Code

serrrfirat and others added 2 commits July 25, 2026 00:10
The capability inventory reported 123/123 tested with zero waivers, but
"tested" was satisfiable three ways and one of them proved nothing:
`_recorded_tool_evidence` credits a capability when any harvested trace
emits a `tool_calls` entry with that name. For a write, a recorded model
response naming `slack__send_message` proves the model chose the tool and
says nothing about whether the provider committed the effect.

Eight `external_write` capabilities rested on that proxy. Auditing them
found six do have provider readback inside `_assert_google_provider_outcome`
/ `_assert_slack_provider_outcome`, but the gate could not see it — deleting
those assertions would have left the inventory green. The other two,
`google-sheets.write_values` and `google-sheets.rename_sheet`, are issued by
qa_7e alongside `append_values` against one spreadsheet whose single asserted
row cannot isolate which call committed it.

Coverage is now counted per capability x outcome class:

- `external_write` capabilities may not rest on a harvested tool-call name.
  They need a typed case with readback, an `integration_evidence` entry, or a
  new `journey_evidence` entry naming the exact test *and* the assertion
  helper that performs the readback. Both symbols are verified to exist, so a
  deleted helper turns the gate red.
- Reads need a seeded `success` case *and* an `empty`-result case, so the
  runtime is proven to distinguish "no results" from "the call failed"
  (epic #6524 workstream 5). Status/transport faults stay with the reusable
  fault profiles; `outcome_class` covers only per-operation semantics.

The read/write split is derived from `external_write` in each tool's
`effects` in the shipped manifests, so a new tool classifies itself.

Whatever does not meet the rules is listed in `coverage_backlog` with owner,
reason, issue, and review condition: 2 writes and 49 reads missing an
empty-result case. The backlog is a ratchet — the gate fails when an entry
names a capability that has since been covered.

Regression coverage: the gate's own teeth are pinned by
`test_journey_evidence_rejects_a_vanished_symbol` and
`test_coverage_backlog_entries_are_owned_and_not_stale`. Manually verified
that dropping a journey_evidence block, pointing an assertion at a deleted
symbol, removing a backlog capability, and backlogging an already-covered
capability each turn the suite red.

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… read

Burns down the coverage_backlog the previous commit made visible, with
executed evidence rather than declarations.

`google-sheets.write_values` and `google-sheets.rename_sheet` were the two
capabilities whose only journey (qa_7e) issues them alongside `append_values`
against one spreadsheet, so a single asserted row could not attribute the
mutation. Both now have typed cases that isolate them:

- write_values pins the exact Sheet1!A4:C4 range, below the seeded rows, so an
  append cannot satisfy it, and re-asserts the seeded row is untouched.
- rename_sheet asserts the same sheetId carries the new title, proving a
  rename rather than a delete-and-recreate.

`google-sheets.read_values` gains the first success/empty pair under the new
outcome dimension. The empty case asserts the positive contract — the model
receives {"range": "Sheet1!A50:C60", "values": []} with status completed —
rather than merely checking that seeded markers did not leak, so it fails if
an empty range ever starts reading back as an error or as stale rows.

The write backlog is now empty; the read backlog drops to 48.

Validation, against the pinned serrrfirat/emulate fork (c85da1e) and a real
`ironclaw serve` process:

- 135 passed, 0 skipped, in 2m52s — 100 provider operation cases, the
  capability inventory gate, and the journey coverage gate.
- Mutation-checked, not just green: replacing write_values' expected cell
  contents fails the case, so the provider readback is load-bearing. The
  empty-read assertion was rewritten after dumping the real payload, because
  the original negative form would have passed on a payload that proved
  nothing.

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@ironloopai

ironloopai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

🔎 IronLoop Review Status

Head: 002bd339ccec73224d9ee616b646a9c3a196bb11
Result: One or more review results were superseded by a newer PR head.
Next: Run @ironloopai review on the latest PR head.
Updated: 2026-07-24T22:19:01.209Z

Current reviewers:

Reviewer State Verdict Findings Last update
ironloop/common-reviewer (reviewer) Superseded N/A N/A 2026-07-24T21:51:06.189Z
Reviewer summaries
Reviewer Detail
ironloop/common-reviewer (reviewer) Superseded by a newer PR head. New head: 6982203. Previous verdict: Changes requested.
Recent activity
Time Reviewer State Detail
2026-07-24T21:36:54.748Z ironloop/common-reviewer (reviewer) Queued Accepted review request for head 74806c9.
2026-07-24T21:36:54.748Z ironloop/common-reviewer (reviewer) Queued Waiting for this reviewer lane to become available.
2026-07-24T21:36:55.150Z ironloop/common-reviewer (reviewer) Started Reviewer worker started.
2026-07-24T21:36:57.836Z ironloop/common-reviewer (reviewer) Workspace ready Prepared isolated checkout (merge_ref) at 7499c7f.
2026-07-24T21:42:31.984Z ironloop/common-reviewer (reviewer) Result captured Changes requested; 2 blocking findings.
2026-07-24T21:42:31.984Z ironloop/common-reviewer (reviewer) Completed Review completed and terminal status was persisted.
2026-07-24T21:51:06.189Z ironloop/common-reviewer (reviewer) Superseded A newer PR head replaced this review (6982203).
Available commands
  • @ironloopai help
  • @ironloopai agents
  • @ironloopai review
  • @ironloopai review --agent <agent>
Run metadata

Admission: webhook accepted the request and IronLoop persisted reviewer state before this projection.

@github-actions github-actions Bot added the scope: docs Documentation label Jul 24, 2026
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6664 July 24, 2026 21:37 Destroyed
@github-actions github-actions Bot added size: XS < 10 changed lines (excluding docs) risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 46ef7b37-22bf-4cdc-b81f-4915528c4b78

📥 Commits

Reviewing files that changed from the base of the PR and between 7359dea and 002bd33.

📒 Files selected for processing (2)
  • tests/e2e/fixtures/provider_capability_coverage.toml
  • tests/e2e/scenarios/test_provider_capability_inventory.py

📝 Walkthrough

Summary by CodeRabbit

  • Testing

    • Expanded end-to-end Google Sheets provider scenarios for write_values, rename_sheet, and read_values, including explicit empty-range reads.
    • Strengthened provider-capability inventory completeness checks using executable evidence and enforcing coverage by capability × outcome class (reads require both success and empty outcomes).
    • Added AST-based validation ensuring “journey evidence” references and assertion helpers are truly executed as intended, plus backlog integrity and non-stale gap checks.
  • Documentation

    • Updated internal guidance and e2e coverage docs/fixtures to reflect the new “tested” and evidence rules, including clarified ratcheting behavior for remaining coverage gaps.

Walkthrough

Provider capability coverage now distinguishes semantic outcomes, requires provider-state evidence for writes, requires success and empty cases for reads, validates executable journey references, and tracks remaining gaps through a ratcheting backlog.

Changes

Provider capability outcome coverage

Layer / File(s) Summary
Typed outcome evidence
tests/e2e/provider_operation_types.py, tests/e2e/provider_operation_google_sheets_cases.py
Adds success and empty outcome classes and Google Sheets cases covering writes, renames, populated reads, and empty reads.
Inventory classification and gate checks
tests/e2e/provider_capability_inventory.py, tests/e2e/scenarios/test_provider_capability_inventory.py
Classifies capabilities by manifest effects and validates executable journey evidence, typed write evidence, required read outcomes, and backlog consistency.
Evidence declarations and documentation
tests/e2e/fixtures/provider_capability_coverage.toml, docs/internal/testing-playbook.md, tests/e2e/CLAUDE.md
Adds journey evidence and backlog entries and documents capability-by-outcome coverage rules.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: think-in-universe

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title uses Conventional Commits style and accurately summarizes the main change.
Description check ✅ Passed The description is detailed and covers summary, linked issue, validation, test strategy, and reviewer notes, with only some template sections omitted.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ IronLoop Review: reviewer

Review at a glance

Verdict Blocking Notes Inline Head
❌ Changes requested 2 1 3 74806c9bf9c5

Head: 74806c9bf9c5fb6de9f2a03ee0877c32c6528bb6
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.

Run details

Status: Current
Needs human: no
Needs validation: no

Summary

Changes requested: the new coverage gate can credit journey-only writes without proving readback is invoked, and it does not ratchet fulfilled read backlog entries. The E2E guidance also reports obsolete backlog totals.

Findings

Blocking: 2 / Notes: 1

Blocking findings

1. ❌ [MEDIUM] Require journey evidence to invoke provider readback

Location: tests/e2e/scenarios/test_provider_capability_inventory.py:226-236
This validates only that two def declarations exist. Since JOURNEY_EVIDENCE_CAPABILITY_IDS is later accepted as complete write coverage, deleting the await _assert_*_provider_outcome(...) invocation from the named parametrized test would leave both symbols and recorded tool names intact, so all six journey-only writes would still pass without provider-side readback. Bind each capability to a concrete journey/readback execution (or at least verify the test invokes the helper) and add a negative case for an uncalled helper.

2. ❌ [MEDIUM] Ratchet fulfilled read backlog entries

Location: tests/e2e/scenarios/test_provider_capability_inventory.py:346-352
The stale-entry check skips every read_requires_outcome_classes entry. A backlogged read can gain both success and empty cases yet remain in coverage_backlog indefinitely while this test passes, leaving the reported gap stale and hiding completed coverage. For read entries, fail when REQUIRED_READ_OUTCOME_CLASSES is a subset of the capability's covered outcomes.

Non-blocking notes (1)
1. 💬 [LOW] Update the documented backlog totals

Location: tests/e2e/CLAUDE.md:268-272
This is stale after the second PR commit: the fixture now has no write backlog entries and exactly 48 read entries. The two named Sheets writes have typed cases in this PR, so the guidance should state 0 writes and 48 reads.

Developer follow-up

After fixing this feedback:

  1. Push the fix to this PR branch.
  2. Re-run this reviewer with @ironloopai review --agent reviewer if you only changed this reviewer's findings.
  3. Re-run all reviewers with @ironloopai review when the fix may affect multiple areas.

Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py Outdated
Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py Outdated
Comment thread tests/e2e/CLAUDE.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@tests/e2e/CLAUDE.md`:
- Around line 247-264: Correct the ratchet documentation for
read_requires_outcome_classes so it does not claim the gate fails when an entry
names an already-covered capability/outcome. Align the enforcement description
with the actual test-suite behavior while preserving the documented requirements
for read and external_write evidence.
- Around line 265-274: Update the backlog snapshot paragraph near
`coverage_backlog` to match the PR’s final state: report zero uncovered write
capabilities, remove the outdated `google-sheets.rename_sheet` and
`google-sheets.write_values` examples, and report 48 read capabilities missing
empty-result coverage, consistent with `provider_capability_coverage.toml`.

In `@tests/e2e/scenarios/test_provider_capability_inventory.py`:
- Around line 323-353: Extend
test_coverage_backlog_entries_are_owned_and_not_stale so
read_requires_outcome_classes entries are also rejected when their capabilities
have fully covered REQUIRED_READ_OUTCOME_CLASSES, while preserving the existing
write_requires_operation_case check. In docs/internal/testing-playbook.md lines
459-493 and tests/e2e/CLAUDE.md lines 247-274, no direct changes are required;
their generic ratchet claims become enforced by the test update.
🪄 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: aface591-0b19-47e1-9ed4-68f7a1fd5286

📥 Commits

Reviewing files that changed from the base of the PR and between e074a39 and 74806c9.

⛔ Files ignored due to path filters (1)
  • tests/e2e/ironclaw_e2e.egg-info/SOURCES.txt is excluded by !tests/e2e/ironclaw_e2e.egg-info/**
📒 Files selected for processing (7)
  • docs/internal/testing-playbook.md
  • tests/e2e/CLAUDE.md
  • tests/e2e/fixtures/provider_capability_coverage.toml
  • tests/e2e/provider_capability_inventory.py
  • tests/e2e/provider_operation_google_sheets_cases.py
  • tests/e2e/provider_operation_types.py
  • tests/e2e/scenarios/test_provider_capability_inventory.py

Comment thread tests/e2e/CLAUDE.md
Comment thread tests/e2e/CLAUDE.md
Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py Outdated
…eads

Addresses review on #6664.

1. Journey evidence checked only that the test and the assertion helper were
   *declared*. Deleting the `await _assert_*_provider_outcome(...)` call would
   leave both symbols and the recorded tool names intact, so a journey-only
   write stayed credited with no readback running — the same
   evidence-decoupled-from-measurement bug this PR exists to fix. The gate now
   extracts the named test's body and requires the assertion helper to be
   invoked inside it.

2. The staleness ratchet only covered `write_requires_operation_case`, so a
   backlogged read that had since gained both required outcome classes could
   sit in the backlog forever while both docs promised otherwise. It now also
   rejects read entries whose capability covers every required outcome class.

3. tests/e2e/CLAUDE.md still reported the pre-burn-down totals (2 writes, 49
   reads). Corrected to zero writes and 48 reads. The playbook's ratchet
   sentence needed no change — it is accurate now that the code enforces both
   rules.

Regression coverage: `test_journey_evidence_rejects_a_declared_but_uncalled_helper`
pins finding 1 by asserting the declaration check passes on the exact source
the invocation check must reject.

Mutation-verified, not just green:
- deleting the readback call while keeping both symbols -> fails
- leaving a fully-covered read capability in the backlog -> fails
- baseline restores to 11 passed

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6664 July 24, 2026 21:51 Destroyed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/e2e/scenarios/test_provider_capability_inventory.py`:
- Around line 239-247: Replace the raw-text regex check in
_assert_python_symbol_called with Python AST parsing of body, and detect an
executable call to the named symbol rather than comments or string contents.
Preserve the existing assertion failure message and caller/source_label context
while ensuring declaration-only or textual mentions do not satisfy the check.
🪄 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: dcdb8218-9c18-465a-8e36-d43dd0372d5e

📥 Commits

Reviewing files that changed from the base of the PR and between 74806c9 and 6982203.

📒 Files selected for processing (2)
  • tests/e2e/CLAUDE.md
  • tests/e2e/scenarios/test_provider_capability_inventory.py

Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py
Addresses review on #6664.

`_assert_python_symbol_called` regex-searched the raw function body for
`symbol(`, so a commented-out call — `# await _assert_readback(url)` — or the
same text inside a string literal satisfied it while the helper never ran.
That re-admits precisely the declaration-only evidence the previous commit was
written to reject; this is the third variant of the same
evidence-decoupled-from-execution bug in this PR, which is a good argument for
checking structure instead of text.

The gate now parses the evidence source with `ast`, locates the named
module-level test, and walks it for an `ast.Call` to the named helper. AST
nodes only represent executable code, so comments and string contents cannot
satisfy the check by construction. `_python_function` replaces both the
declaration regex and the line-slicing body extractor.

Regression coverage: `test_journey_evidence_rejects_a_helper_that_never_runs`
is now parametrized over uncalled, commented-out, and inside-a-string bodies,
each asserting the declaration check still passes so the reader can see exactly
which hole the call check closes. `test_journey_evidence_accepts_a_genuinely_
invoked_helper` guards the other direction so the check cannot be tightened
into always-fail.

Mutation-verified against the real journey test, not just synthetics:
- commenting out `await _assert_google_provider_outcome(...)` -> fails
- deleting the call outright -> fails
- baseline restores to 14 passed

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/e2e/scenarios/test_provider_capability_inventory.py`:
- Around line 226-239: Add an attribute-call case to the positive test for
_assert_python_symbol_called, invoking the target symbol through a receiver so
the ast.Attribute branch is exercised. Update the test docstring or description
to accurately state that both direct and attribute-based calls are covered,
while preserving the existing direct-call assertion.
🪄 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: 06a2b38d-408f-4f5c-99e8-b342a38ae869

📥 Commits

Reviewing files that changed from the base of the PR and between 6982203 and 2f797c1.

📒 Files selected for processing (1)
  • tests/e2e/scenarios/test_provider_capability_inventory.py

Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py Outdated
Addresses review on #6664.

The positive test's docstring claimed coverage "including via a wrapper" while
its body only exercised a direct `ast.Name` call, leaving the `ast.Attribute`
branch — the looser of the two, since it matches any receiver — untested. An
overstated claim with nothing behind it is the same defect this PR exists to
remove, so it is worth fixing rather than rewording.

Parametrized over a direct call and a call through a receiver, with the
receiver-agnostic behavior noted at the match site so a future tightening is a
visible decision rather than a silent one. The docstring now also says why the
positive case exists: a detector that rejected everything would make the gate
unfalsifiable rather than strict.

Mutation-verified: deleting the `ast.Attribute` clause fails the new
"through a receiver" case. 40 passed across both fast gates.

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6664 July 24, 2026 22:03 Destroyed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/e2e/scenarios/test_provider_capability_inventory.py`:
- Around line 351-360: Remove the unused label parameter from
test_journey_evidence_accepts_a_genuinely_invoked_helper while preserving
readable parametrized case IDs, either by renaming it to _label or by moving the
labels into pytest.param id values.
🪄 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: dd765d16-ee8c-4f72-a777-7caf2b3b4574

📥 Commits

Reviewing files that changed from the base of the PR and between 2f797c1 and 5cc47c2.

📒 Files selected for processing (1)
  • tests/e2e/scenarios/test_provider_capability_inventory.py

Comment thread tests/e2e/scenarios/test_provider_capability_inventory.py Outdated
Addresses review on #6664.

Both parametrized journey-evidence tests carried a `label` argument used only
to name the case, which Ruff flags as ARG001. Moved the labels into
`pytest.param(..., id=...)` rather than renaming to `_label`: the case IDs stay
readable and the dead argument goes away entirely.

The same dead argument existed in the sibling negative test
(`test_journey_evidence_rejects_a_helper_that_never_runs`), which the review
did not name; fixed both.

Verified the IDs are unchanged — uncalled / commented out / inside a string /
direct / through a receiver all still appear in collection — 40 passed across
both fast gates, and `ruff check --select ARG,F401,F841` is clean on the file.

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6664 July 24, 2026 22:08 Destroyed
@railway-app

railway-app Bot commented Jul 24, 2026

Copy link
Copy Markdown

🚅 Deployed to the ironclaw-pr-6664 environment in ironclaw-ci-preview

Service Status Web Updated (UTC)
ironclaw 🕒 Building (View Logs) Web Jul 24, 2026 at 10:08 pm

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
tests/e2e/scenarios/test_provider_capability_inventory.py (3)

207-223: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Resolve the runtime-bound test binding, not the first AST match.

_python_function returns the first matching module-level definition, but later duplicate definitions shadow earlier ones in the test module. As written, stale journey evidence can still pass if an earlier test_journey calls the readback helper while pytest executes a later redefinition that does not. Reject duplicate module-level symbols or resolve the final binding, and add a regression case.

🤖 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/e2e/scenarios/test_provider_capability_inventory.py` around lines 207 -
223, The _python_function helper must reflect Python’s runtime binding when
duplicate module-level definitions exist. Reject duplicate matching symbols or,
preferably, resolve and return the final matching FunctionDef/AsyncFunctionDef
so checks inspect the definition pytest executes; add a regression case covering
an earlier stale definition followed by a later redefinition.

Source: Path instructions


414-424: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Keep integration evidence from bypassing read outcome coverage.

tests/e2e/CLAUDE.md and provider_capability_coverage.toml treat reads as capability × outcome class, requiring both success and empty. test_read_capabilities_cover_every_required_outcome_class() subtracts INTEGRATION_EVIDENCE_CAPABILITY_IDS, an untyped capability set, so a read can miss either required outcome class. Remove that subtraction from the read gate or make integration evidence outcome-aware before exempting reads.

🤖 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/e2e/scenarios/test_provider_capability_inventory.py` around lines 414 -
424, Update test_read_capabilities_cover_every_required_outcome_class so
integration evidence cannot exempt capabilities from read outcome coverage;
remove INTEGRATION_EVIDENCE_CAPABILITY_IDS from the READ_CAPABILITY_IDS
subtraction, or replace it with outcome-aware evidence that verifies each
required outcome class. Preserve the existing success-and-empty checks and
missing-capability reporting.

Source: Path instructions


226-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Require an actually executed journey readback call.

ast.walk(function) also accepts calls declared inside nested/async function definitions and async helper calls without await. As tests/e2e/CLAUDE.md requires executable provider readback for external_write, these don’t satisfy the invariant. Walk only the body/scoping-reachable execution path and require the coroutine to be awaited before accepting the helper as executable evidence. Add regression cases for nested-uninvoked and unawaited coroutine calls.

🤖 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/e2e/scenarios/test_provider_capability_inventory.py` around lines 226 -
239, Update _assert_python_symbol_called to inspect only the current function’s
executable body, excluding nested function or async definitions, and accept
async helper calls only when they are awaited. Preserve detection of direct and
attribute-based symbol calls, and add regression cases covering an uninvoked
nested call and an unawaited coroutine call.

Source: Path instructions

🤖 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 `@tests/e2e/scenarios/test_provider_capability_inventory.py`:
- Around line 207-223: The _python_function helper must reflect Python’s runtime
binding when duplicate module-level definitions exist. Reject duplicate matching
symbols or, preferably, resolve and return the final matching
FunctionDef/AsyncFunctionDef so checks inspect the definition pytest executes;
add a regression case covering an earlier stale definition followed by a later
redefinition.
- Around line 414-424: Update
test_read_capabilities_cover_every_required_outcome_class so integration
evidence cannot exempt capabilities from read outcome coverage; remove
INTEGRATION_EVIDENCE_CAPABILITY_IDS from the READ_CAPABILITY_IDS subtraction, or
replace it with outcome-aware evidence that verifies each required outcome
class. Preserve the existing success-and-empty checks and missing-capability
reporting.
- Around line 226-239: Update _assert_python_symbol_called to inspect only the
current function’s executable body, excluding nested function or async
definitions, and accept async helper calls only when they are awaited. Preserve
detection of direct and attribute-based symbol calls, and add regression cases
covering an uninvoked nested call and an unawaited coroutine call.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1523a3c4-49be-44b3-a077-a5d0a96d396d

📥 Commits

Reviewing files that changed from the base of the PR and between 5cc47c2 and 7359dea.

📒 Files selected for processing (1)
  • tests/e2e/scenarios/test_provider_capability_inventory.py

… gates

Addresses three review findings on #6664, all the same shape: the check did
not match what actually executes.

1. `_python_function` returned the first matching module-level def, but Python
   binds the last. A stale earlier `test_journey` calling the readback could
   vouch for a later redefinition that does not. Duplicates are now rejected
   outright — a duplicate module-level name is a defect in its own right, so
   picking either one silently would be wrong.

2. `ast.walk` descended into nested `def`/`lambda`/`class` bodies, so a call
   inside a nested function nobody invokes counted as evidence, and an
   un-awaited coroutine call counted even though it builds a coroutine and
   discards it without running a single assertion. Traversal now stays in the
   function's own execution scope, and when the helper is an
   `AsyncFunctionDef` the call must be awaited. The helper node is passed in
   rather than its name so async-ness is known rather than guessed.

3. The read gate subtracted `INTEGRATION_EVIDENCE_CAPABILITY_IDS`, silently
   exempting four read capabilities from the empty-result rule. Integration
   evidence names one executable Cargo test per capability and carries no
   outcome class, so it cannot stand in for outcome coverage — and a silent
   exemption is exactly what this gate exists to remove. The subtraction is
   gone; the four are now backlogged with an owner and the real blocker (no
   provider world in which to seed an empty result at a local-WASM, hosted-MCP,
   or Exa-backed seam). Read backlog: 48 -> 52, honestly stated.

Regression coverage: unawaited-coroutine, call-inside-uninvoked-nested-def, and
duplicate-definition cases, alongside the existing uncalled / commented-out /
inside-a-string ones.

Mutation-verified against the real journey test:
- removing `await` while keeping the call -> fails
- moving the call into an uninvoked nested def -> fails
- appending a duplicate test definition -> fails
- un-backlogging an integration-evidence read -> fails
- baseline restores to 18 passed; 43 across both fast gates; ruff clean

Note: the first attempt at the await mutation silently did not apply (wrong
indentation) and looked like a pass. Re-run with an assertion that the
substitution actually changed the file — a mutation that does not apply proves
nothing.

Refs #6524

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to ironclaw-ci-preview / ironclaw-pr-6664 July 24, 2026 22:19 Destroyed
@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Addressed all three outside-diff findings from the last review in 002bd339c. They were the same shape — the check did not match what actually executes:

  1. First-def vs runtime binding. _python_function returned the first module-level match while Python binds the last, so a stale earlier test_journey could vouch for a redefinition that never calls the readback. Duplicates are now rejected outright rather than resolved to the last, since a duplicate module-level name is a defect in its own right and silently picking either one would be wrong.

  2. ast.walk over-accepted. It descended into nested def/lambda/class bodies, so a call inside a function nobody invokes counted; and an un-awaited coroutine call counted even though it builds a coroutine and discards it without running an assertion. Traversal now stays in the function's own execution scope, and when the helper is an AsyncFunctionDef the call must be awaited. The helper node is passed in rather than its name, so async-ness is known rather than guessed.

  3. Integration evidence silently exempted reads. You were right that this is untyped: it names one Cargo test per capability with no outcome class. Four reads (github.handle_webhook, nearai.web_search, web-access.get_content, web-access.search) were skipping the empty-result rule invisibly. Removed the subtraction and backlogged them with the real blocker — no provider world in which to seed an empty result at a local-WASM, hosted-MCP, or Exa-backed seam. Read backlog 48 → 52, which is the honest number.

New regression cases: unawaited-coroutine, call-inside-uninvoked-nested-def, duplicate-definition, alongside the existing uncalled / commented-out / inside-a-string.

Mutation-verified against the real journey test, each turning the gate red: removing await while keeping the call; moving the call into an uninvoked nested def; appending a duplicate test definition; un-backlogging an integration-evidence read. Baseline 18 passed, 43 across both fast gates, ruff clean.

One process note worth recording: my first attempt at the await mutation silently did not apply — wrong indentation — and reported a pass, which I nearly read as "the gate handles this". I now assert the substitution actually changed the file before trusting the result. A mutation that does not apply proves nothing, which is the same failure mode as the evidence bugs this PR is about.

🤖 Addressed by Claude Code

@serrrfirat
serrrfirat merged commit 44d3710 into main Jul 24, 2026
44 checks passed
@serrrfirat
serrrfirat deleted the firat/capability-outcome-coverage-gate branch July 24, 2026 22:33

This branch was successfully deployed

No deployments
ironclaw-ci-preview / ironclaw-pr-6664 — 002bd339 Deployed Jul 24, 2026 by railway-app[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: docs Documentation size: XS < 10 changed lines (excluding docs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant