Skip to content

fix(browser): bind the Press action before borrowing it in the approval-target test - #7879

Merged
senamakel merged 1 commit into
tinyhumansai:mainfrom
senamakel:fix-browser-test-temp
Oct 15, 2026
Merged

senamakel merged 1 commit into
tinyhumansai:mainfrom
senamakel:fix-browser-test-temp

Conversation

@senamakel

Copy link
Copy Markdown
Member

Summary

#7863 (tinycomputer v0.14.0 bump) added a test that does approval_target(&Action::Press { .. }). approval_target returns an Option<&str> that borrows from the action, and the temporary Action is dropped at the end of the statement. So cargo check -p openhuman --all-targets fails on main (8ca0456) with E0716, which breaks every lib-test build.

This binds the action to a local first. Test-only, no behaviour change.

Tests

On upstream/main 8ca0456:

  • cargo check -p openhuman --all-targets and --features e2e-test-support --all-targets both compile.
  • cargo test -p openhuman --lib browser::: 61 passed.

Refs #7801, #7523

Restructure the press approval assertion to bind the action to a local
variable before passing it to approval_target, avoiding a temporary that
outlives the borrow.

Auto-committed-on: dragonfly
@tinysweeper

tinysweeper Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper completed its review; deterministic results follow.

State: Ready for maintainer review
Priority: none
Reviewed head: d30f9ea16384
Updated: 2026-10-15T14:58:11Z

Review snapshot

Change surface Files Review signal Count
Production 0 Active findings 0
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

How this fits together

flowchart LR
  n0["...al_describes_selector_and_locator_targets<br/>changed"]:::changed
  n1["session_entry"]:::impacted
  n2["format"]:::impacted
  n3["Role"]:::impacted
  n4["idle_and_oldest_sessions_are_bounded"]:::impacted
  n5["approval_target"]:::impacted
  n0 -->|uses| n3
  n0 -->|calls| n5
  n0 -->|tests| n5
  n4 -->|calls| n1
  n4 -->|tests| n1
  n4 -->|calls| n2
  n4 -->|tests| n2
  n5 -->|calls| n2
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The refactor is behaviorally identical: `approval_target` still receives the same `Action::Press` variant, and the assertions and the untouched untargeted `Action::Press` literal are identical on both sides of the change, with no other test or caller affected.
  • Lane summary: The diff is a pure mechanical refactor of one test: it binds `Action::Press { key: "Enter", target: Some(Target::reference("e7")) }` to `press_action` first and then calls `approval_target(&press_action)`, rather than calling `approval_target` on the inline literal. The produced tuple is unchanged — `approval_target` (browser_pending.rs:32) still receives the same `Action::Press` variant, and the assertions that follow (`assert_eq!(reference, Some("e7")); assert_eq!(press, " @e7");`) and the untouched `Action::Press { .. }` literal just below are identical on both sides of the change. The lookup confirms `approval_target` takes `&Action` and for `Press { target, .. }` returns `target.as_ref()` and the ` @e7` ref string, so the new code is behaviorally identical to the old. There is nothing in the diff that fails, misbehaves, regresses, or violates a repository rule; no other test or caller is affected. I have no findings — the change looks sound as a refactor that only binds a variable before an existing call, and the only context I could not see (the full test module) contains nothing the touched lines depend on that the lookup and surrounding context have not already established for these exact lines. _The code index is behind this pull request (indexed at `d252e90bb423`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The diff is a mechanical test refactor: binding `Action::Press { ... }` to a named `press_action` before calling `approval_target(&press_action)` instead of calling it inline. The lookup of `approval_target` confirms `Action::Press { target, .. } => target.as_ref()` matches a struct-literal argument the same way whether bound to a variable or passed inline, so behaviour and assertions (`Some("e7")`, `" @e7"`) are unchanged. No untrusted input path, no auth/authorisation change, no new calls, no rule from the repository's instruction files is violated (the test file already follows the sibling-tests convention). Nothing reportable found; the change looks sound. Looking at the diff only, this is a pure mechanical refactor of a test: the `Action::Press { ... }` struct literal that was previously constructed inline as an argument to `approval_target` is now bound to a local variable `press_action` first, and the call site changed to `approval_target(&press_action)`. Nothing about the action's shape, the target, or the assertions changes. The lookups confirm `approval_target` handles `Action::Press { target, .. } => target.as_ref()`, which is unchanged. No security-relevant behaviour, no untrusted input path, no auth change, no subprocess or network call, no serialization, no path traversal, no secrets, no CI/permission changes, no dependency change. The repository's own rules require Rust unit tests to be non-inline in a sibling `<module>_tests.rs` — this file is already such a sibling test module (declared via `#[path = "browser_computer_tests.rs"]`), and the diff does not violate that rule. No finding to report; the change looks sound. _The code index is behind this pull request (indexed at `d252e90bb423`), so retrieved context may be out of date._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The assertions (`reference == Some("e7")`, `press == " @e7"`, and the untargeted case) are unchanged and can still fail on regression, and no behavior outside tests moved, so no new test is owed.
  • Lane summary: The only change is a mechanical extraction in a test: the `Action::Press` literal previously passed inline to `approval_target` is now bound to `press_action` and passed by reference instead. Semantics are identical, the assertions (`reference == Some("e7")`, `press == " @e7"`, untargeted case) are unchanged and can still fail on regression, and no behaviour outside tests moved — so no new test is owed. The change looks sound. _The code index is behind this pull request (indexed at `d252e90bb423`), so retrieved context may be out of date._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The pull request description accurately matches the diff: the E0716 temporary-lifetime diagnosis is correct because `approval_target` (browser_pending.rs:32) returns an `Option<&str>` borrowing from the action, and the unchanged untargeted call is correctly unaffected since its reference is destructured to `_` within the same statement.
  • Lane summary: The change is a small, accurate test-only fix: `approval_target` (browser_pending.rs:32) returns `(Option<&str>, String)` borrowing from the action, and the previous code bound that reference to `reference`, which outlives the temporary `Action` — E0716 as described. Binding the action to `press_action` first fixes it. The unchanged `let (_, untargeted) = approval_target(&Action::Press { ... });` on the following line is not affected, because the `Option<&str>` element is destructured to `_` within the same statement and so never outlives the temporary. The description matches the diff, the fix is test-only with no behaviour change, and no repository rule is implicated (the test already lives in the sibling `browser_computer_tests.rs` file). _The code index is behind this pull request (indexed at `d252e90bb423`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, glm-5.3-flash
  • Spend: $0.002070
  • Tokens: 51535 input · 5359 output · 0 cached · 49 embedding
Head State Pass summary
d30f9ea16384 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-15T14:58:11Z)

tinysweeper 0.1.2

@senamakel
senamakel merged commit 33032a7 into tinyhumansai:main Oct 15, 2026
7 checks passed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T14:57:13.148391Z d30f9ea PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8c16edcc-3903-4c72-8f61-dd5f5cf1ee60
📥 Commits

Reviewing files that changed from the base of the PR and between 8ca0456 and d30f9ea.

📒 Files selected for processing: 1
  • crates/openhuman-core/src/tools/impl/browser/browser_computer_tests.rs
+-----------------------------------------------------------------------------------+
| The best way to predict the future is to implement it. - David Heinemeier Hansson |
+-----------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@tinysweeper tinysweeper 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.

tinysweeper found nothing blocking. Approving.

             $0.0021 · 51,535 in / 5,359 out · 0 cached (0%) · ladder/vectors-oai3, glm-5.3-flash · 49 embedded
critique:    $0.0001 · 13,712 in / 292 out   · 0 cached (0%) · glm-5.3-flash
security:    $0.0001 · 13,468 in / 373 out   · 0 cached (0%) · glm-5.3-flash
tests:       $0.0006 · 12,263 in / 320 out   · 0 cached (0%) · glm-5.3-flash
description: $0.0010 · 9,931 in  / 3,360 out · 0 cached (0%) · glm-5.3-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant