Fix all open Codex recovery-capture findings on PR #4 - #5
Conversation
Exercise environment-array exposure in page callbacks, future and nested iframe scopes, sensitive value getters, and existing fail-closed expectations through the real page worker. Use only local fixtures and synthetic environment values.
Keep environment-value comparisons in Node instead of passing secrets into page-realm evaluations. Reject protected identities and sensitive controls before reading value getters, retaining Playwright's atomic shadow-aware protection. Resolve only currently present recovery/protected frame scopes without waits; preserve strict malformed/ambiguous/action/check handling. Skip capture entirely without recovery fields, and re-evaluate protection when later frames appear.
|
Review changes with Changed Files
|
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f1a0f4b46
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Apply the recovery capture fixes and regression coverage from stacked PR #5 (source commit 50889f2) directly to PR #4. - Keep environment-secret comparisons in Node, outside page evaluations. - Skip absent future frame scopes during passive capture and bypass empty recovery allowlists without relaxing required action or assertion scopes. - Reject protected identities and sensitive controls before reading values, preserving pinned-node and shadow-aware checks. - Cache protected frame traversal within each recovery snapshot. - Include 21 real-Chrome regressions in the browser command and CI. Addresses Codex review comments 4061550418, 4061550427, and 4061578096.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50889f2bb8
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Assert that eight recovery fields and twenty protected steps over two frame paths require only three path lookups per snapshot, and that the next snapshot resolves afresh. Keep the legacy visible=false result for an absent top-level control and cover it alongside strict missing-frame checks.
Persist a recovery-value capture suspension before an environment-backed UI dispatch. Current selectors and exact value equality cannot prove a transformed or rerendered credential became public. Preserve prior snapshots and explicit non-secret input intents, while preventing later automatic observations from replacing them. Honor the same durable boundary in managed Jev, including across reattachment, reconstruction, missing environment values and legacy Playwright receipts without provenance. Keep protected-alias preflight checks active in the scoped adapter. Add eight real-Chrome formatter/mirror/reconstruction tests, cross-language handoff assertions, and four Python policy tests. No new dependencies or documentation. Addresses PR #4 comment 4072551621.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1438b9c9d9
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const legacy = steps.some(s => s.valueFromEnv !== undefined) && events.some(e => | ||
| e.type === "action.start" && e.data.engine === "playwright" && e.data.recoveryGuarded !== true); | ||
| if (suspended || legacy) { | ||
| if (!suspended) suspendRecoveryCapture(journal); | ||
| save = false; // Still reject known protected aliases, but never read new page values. |
There was a problem hiding this comment.
When resuming a run created by the previous implementation, an unguarded environment-backed fill may already have completed its post-action capture and written a transformed or mirrored secret into recovery.json. This branch merely sets save = false, so that untrusted value remains persisted and can later be consumed through valueFromRecovery; detecting a legacy boundary must quarantine or remove the existing recovery payload rather than only preventing future captures.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
Adopt the existing, CI-validated conservative safeguard from PR #5 head 1438b9c, preserving this branch's added visibility regressions and documenting the recovery tradeoff. Persist a capture-suspended boundary before environment-backed dispatch; honor it across resume, reconstruction, Jev and Midscene. Prior snapshots and explicit non-secret Playwright fill intents remain usable. Do not try to track transformed secrets through node replacements or page formatters. This supersedes the initial deferral in cb060b2: the concurrent PR supplies a small fail-closed policy rather than a taint-tracking subsystem. Include its browser/Python regressions and keep dependencies unchanged. Addresses Codex review comment 4072551621.
1 participant
Runtime fixes and regression tests for the open Codex comments on PR #4. Targets PR #4's branch, not
main; merge #5 before #4. Current head:1438b9c9d9a8b97e73aec8782867597703135875.PR #4 concurrently copied our earlier fixes into
b410efc. This branch incorporates that commit without discarding changes and is conflict-free.visible=falsefor an absent control, without relaxing required frame scopes.Full CI is green: run 35740049668. Across this work, 30 added real-Chrome cases and four added Python tests pass, together with existing Jev/Midscene, crash-recovery, MCP-image, alias/rerender, unit/protocol and audit checks. The original tests-only run reproduced 16 failures before the first fixes.
Every review thread has a fix/evidence reply. PR #4's two newest threads remain open until these fixes reach that branch; both follow-ups on #5 are resolved. No runtime dependencies, Action pins, operational skills or design docs changed. No production accounts or paid-model calls. This is targeted recovery protection, not general secret redaction or cleanup of previously saved files.