Skip to content

feat(browser): persist thinking-effort selection evidence - #397

Open
frontierkodiak wants to merge 3 commits into
steipete:mainfrom
frontierkodiak:thinking-effort-evidence
Open

feat(browser): persist thinking-effort selection evidence#397
frontierkodiak wants to merge 3 commits into
steipete:mainfrom
frontierkodiak:thinking-effort-evidence

Conversation

@frontierkodiak

Copy link
Copy Markdown
Contributor

What

ensureThinkingTime verified the requested effort tier and then threw the result away, so a completed run left no machine-checkable proof of the tier it submitted at — only an unstructured [browser] Thinking time: … log line.

That gap is load-bearing rather than cosmetic. browser.modelSelection cannot stand in for it: for a Pro-capable target the picker deliberately reports the requested model string as the resolved label, so

resolvedLabel === requestedModel; verified: true

is byte-identical whether or not the Pro effort row was ever selected. Anything asking "did this run answer at Pro effort?" had nothing to read.

Second, sanitizeResult was dropping the answer to that question at the bridge. Its whitelist correctly keeps host detail (pids, ports, profile paths) from crossing to a client on another machine, but it had drifted narrower than that rule and also dropped modelSelection, thinkingSelection, archive, tabUrl, conversationId, and promptSubmitted. A remote caller could not tell which model or effort answered their prompt, nor bind the answer to a durable ChatGPT URL — and silently, since the fields are optional.

Approach

ensureThinkingTime returns a BrowserThinkingSelectionEvidence record alongside its existing logging; it is persisted as browser.thinkingSelection parallel to browser.modelSelection, crosses the bridge on BrowserRunResult, and renders in oracle status.

verified is true only for the two statuses that positively observed the option's selected state (aria-checked/selected/data-state, or a composer pill matching the target tier). Strict fail-closed requests still throw before submit on every other outcome, so they never produce an unverified record — which makes a persisted strict record proof by refusal that no degraded tier was used.

The added sanitizeResult fields describe the conversation, not the host; the three that describe the host stay explicitly undefined, and the test now asserts both halves of the rule so a future field cannot quietly pick the wrong side.

Real behavior

A live Sol run at Pro effort now records:

[browser] Model selection evidence: requestedKey=gpt-5.6-sol; target=GPT-5.6 Sol; resolvedLabel=GPT-5.6 Sol; status=already-selected; verified=yes
[browser] Thinking effort evidence: requestedLevel=pro; status=switched; resolvedLabel=Pro; verified=yes; failClosed=yes

The second line is the one that was previously unavailable. On a --followup, model selection is legitimately reported skipped/verified=no (a resumed conversation keeps its model) while effort evidence still reads verified=yes — the record stays honest about which of the two it re-proved.

Tests

Four added covering the evidence contract, including that a strict Pro request never resolves to an unverified record across every degraded picker outcome. Two existing assertions that pinned the old void return were updated to assert the evidence shape instead. Bridge test asserts evidence and identity cross while host detail does not.

Full suite green: 1766 passed / 43 skipped.

`ensureThinkingTime` verified the requested effort tier and then threw the
result away, so a completed run left no machine-checkable proof of the tier it
submitted at — only an unstructured `[browser] Thinking time: …` log line.

That gap is load-bearing rather than cosmetic. `browser.modelSelection` cannot
stand in for it: for a Pro-capable target the picker deliberately reports the
requested model string as the resolved label, so
`resolvedLabel === requestedModel; verified: true` is byte-identical whether or
not the Pro effort row was ever selected. Any consumer asking "did this run
answer at Pro effort?" had nothing to read.

`ensureThinkingTime` now returns a `BrowserThinkingSelectionEvidence` record
alongside its existing logging, and it is persisted as `browser.thinkingSelection`
parallel to `browser.modelSelection`, crossing the remote bridge on
`BrowserRunResult` and rendering in `oracle status`.

`verified` is true only for the two statuses that positively observed the
option's selected state. Strict (fail-closed) requests still throw before submit
on every other outcome, so they never produce an unverified record — which makes
a persisted strict record proof by refusal that no degraded tier was used.

Claude-Session: https://claude.ai/code/session_01HsXirqcfqtr1Cae9zYCLDk
…ridged results

`sanitizeResult` whitelists what crosses the bridge, which is the right shape —
a result must never carry this host's pids, ports, or profile paths to a client
on another machine. But the whitelist had drifted narrower than that rule: it
also dropped `modelSelection`, `thinkingSelection`, `archive`, `tabUrl`,
`conversationId`, and `promptSubmitted`.

The effect is that a remote caller could not tell which model or effort answered
their prompt — the evidence the picker verified was computed, logged on the host,
and then discarded at the boundary — and could not bind the answer to a durable
ChatGPT URL. A bridged answer arrived unattributable, and silently so: the fields
are optional, so nothing failed.

None of the added fields describes the host. The three that do stay explicitly
undefined, and the test now asserts both halves of the rule so a future field
cannot quietly pick the wrong side.

Claude-Session: https://claude.ai/code/session_01HsXirqcfqtr1Cae9zYCLDk
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 18, 2026
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 9:33 PM ET / August 31, 2026, 01:33 UTC.

ClawSweeper review

What this changes

The PR persists and displays the selected ChatGPT thinking-effort tier for browser runs and returns associated conversation metadata through the remote result bridge.

Merge readiness

⚠️ Needs maintainer review before merge - 2 items remain

The effort-evidence change remains absent from current main and is a coherent extension of existing browser model evidence. This branch is merge-conflicted with newer remote security work, so it needs a careful rebase before merge.

Priority: P3
Reviewed head: 1564899fad69028521b77aed2e24f344a6169aa9

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The behavior proof and focused tests are credible, but the dirty branch needs a safety-preserving rebase before final patch quality can be assessed.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body supplies a post-change live browser transcript from the changed selector through session logging, showing verified Pro effort evidence; focused tests cover the record contract and remote result propagation.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body supplies a post-change live browser transcript from the changed selector through session logging, showing verified Pro effort evidence; focused tests cover the record contract and remote result propagation.
Evidence reviewed 4 items Current main lacks the evidence record: Current main’s effort selector still returns without a record, while this PR changes it to return typed selection evidence and propagates it through the result.
Merged remote-boundary work: Current main contains the client browser-configuration allowlist from merged PR 398; this branch predates it and GitHub reports the PR as dirty, so the rebase must retain that protection.
Feature history: The existing thinking-effort verification originated in the browser selector and current main later adapted that selector for the direct slider.
Findings None None.
Security None None.

How this fits together

Oracle’s browser runner selects a ChatGPT model and thinking tier before submitting a prompt, then stores run metadata for oracle status and optionally returns it to a remote client. The added record makes the selected effort independently attributable rather than relying on model metadata or logs.

flowchart LR
  A[Browser run configuration] --> B[ChatGPT effort picker]
  B --> C[Selection evidence]
  C --> D[Browser run result]
  D --> E[Session metadata]
  E --> F[Oracle status]
  D --> G[Remote bridge client]
Loading

Before merge

  • Resolve merge risk (P1) - The branch is merge-conflicted with current main; resolution must retain the conversation-scoped browser configuration allowlist from fix(serve)!: accept only conversation-scoped fields from remote clients #398.
  • Complete next step (P2) - The remaining action is conflict-resolution review that must preserve merged remote security behavior, not a discrete automated repair.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +139/-11, tests +173/-2, changelog +10 The evidence record crosses picker logic, result transport, persistence, status display, and two focused test suites.

Merge-risk options

Maintainer options:

  1. Rebase while retaining the remote allowlist (recommended)
    Resolve the remote-server conflict by preserving the merged conversation-scoped configuration allowlist and rerun focused browser and bridge tests.
  2. Pause the combined branch
    Do not merge if the evidence change cannot be cleanly separated from the newer remote-boundary implementation.

Technical review

Best possible solution:

Port the focused evidence record and its browser, status, and bridge coverage onto current main while retaining the already-merged remote configuration boundary.

Do we have a high-confidence way to reproduce the issue?

Not applicable as a conventional bug reproduction: supplied terminal output demonstrates the new evidence record, and source inspection confirms current main still lacks it.

Is this the best way to solve the issue?

Yes, after rebase: a separate typed effort record is more reliable than inferring the selected tier from model-selection metadata or logs.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against bbc1b3b0261d.

Labels

Label changes:

  • add merge-risk: 🚨 compatibility: Browser and remote-server code changed substantially after the PR base, requiring a compatibility-preserving rebase.
  • add merge-risk: 🚨 security-boundary: The rebase touches the remote service boundary that now restricts client-provided browser configuration.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P3: This is a bounded observability and attribution improvement, not a current user-blocking regression.
  • merge-risk: 🚨 compatibility: Browser and remote-server code changed substantially after the PR base, requiring a compatibility-preserving rebase.
  • merge-risk: 🚨 security-boundary: The rebase touches the remote service boundary that now restricts client-provided browser configuration.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies a post-change live browser transcript from the changed selector through session logging, showing verified Pro effort evidence; focused tests cover the record contract and remote result propagation.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies a post-change live browser transcript from the changed selector through session logging, showing verified Pro effort evidence; focused tests cover the record contract and remote result propagation.

Evidence

What I checked:

  • Current main lacks the evidence record: Current main’s effort selector still returns without a record, while this PR changes it to return typed selection evidence and propagates it through the result. (src/browser/actions/thinkingTime.ts:116, bbc1b3b0261d)
  • Merged remote-boundary work: Current main contains the client browser-configuration allowlist from merged PR 398; this branch predates it and GitHub reports the PR as dirty, so the rebase must retain that protection. (src/remote/server.ts:724, 954f13278b42)
  • Feature history: The existing thinking-effort verification originated in the browser selector and current main later adapted that selector for the direct slider. (src/browser/actions/thinkingTime.ts:116, b493dcd06e9b)
  • Submitted behavior proof: The complete PR body includes post-change terminal output with verified Pro effort evidence; the tests also cover the record contract and remote propagation. (1564899fad69)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • frontierkodiak: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase onto current main without losing the PR 398 configuration allowlist, then post focused browser and remote test results from the rebased head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-18T19:04:13.211Z sha 1564899 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-24T20:00:43.015Z sha 1564899 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant