Repository navigation
Conversation
![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: c5cac76f02
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for addressing the sticky-state regression and rejection-message handling. I rechecked the latest head, and the approval transition logic now behaves correctly in both directions.
One compatibility blocker remains. A per-call approval after always_reject is serialized as schema 1.15 with approved=[call_id] and rejected=true. The released v0.20.0 reader accepts schema 1.15 but resolves that call as rejected, so a snapshot written by the updated SDK can be silently mis-resumed by the released reader. Please bump CURRENT_SCHEMA_VERSION, add its summary, and cover backward reading of an existing 1.15 mixed record plus new-schema round-trip and resumed behavior. Please also add explicit same-call exception-reversal tests in both directions.
Once those focused changes and CI are complete, this should be ready for another review.
|
Both fixed. The first was a regression I introduced: a per-call decision that matched the sticky one replaced the bool with a list, so other calls lost the sticky answer. It now only converts when the new decision opposes the sticky value. The second skips the sticky message for a call in the approved list. Other calls still get it. Tests cover both. Verification is green. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the update. I refreshed the latest remote head and checks. All 14 checks are green and the branch is cleanly mergeable, but the head is still 4f20ffaff620 and does not include the requested RunState schema changes or durable-state tests.
The existing changes-requested review therefore remains unresolved. Please bump the schema version, add the schema summary, cover backward reading of the existing 1.15 mixed approval record, and test new-schema round-trip and resumed behavior. Please also add the explicit same-call exception-reversal cases in both directions. Once those changes are pushed and CI passes again, this should be ready for another review.
I may implement a complete solution w/ your contribution credit later.
|
Pushed. Bumped to One test is still red and I did not want to guess at it. |
|
Separate question while I was checking the compatibility direction. Released |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecbedbfcde
ℹ️ 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".
| "Persists canonical tool invocation identity plus sanitized mount authority and trusted " | ||
| "rebind metadata, durable pending input, and resumable next-model-call state." | ||
| ), | ||
| "1.16": "Lets a per-call approval decision override a sticky one for the same tool.", |
There was a problem hiding this comment.
Because SUPPORTED_SCHEMA_VERSIONS is derived from SCHEMA_VERSION_SUMMARIES, adding 1.16 here makes the compatibility corpus test expect matching versions and features entries for 1.16; tests/fixtures/run_state/sources.json still only lists schemas through 1.15, so test_historical_fixture_corpus_matches_supported_schema_versions will fail whenever that test module runs. Please add the minimal/feature fixtures and source metadata for the new durable schema version, or avoid advertising 1.16 as supported until the corpus is updated.
Useful? React with 👍 / 👎.
| context._rebuild_approvals( | ||
| context_data.get("approvals", {}), | ||
| per_call_overrides_sticky=(schema_major, schema_minor) >= (per_call_major, per_call_minor), | ||
| ) |
There was a problem hiding this comment.
This compatibility switch only rebuilds the string-key context["approvals"] records with pre-1.16 sticky precedence; immediately below, hosted_mcp_approvals are restored via _rebuild_hosted_mcp_approvals() which still calls _restore_approval_record() with the default new per-call precedence. A schema 1.14/1.15 snapshot containing a hosted MCP sticky server-tool approval plus a per-call rejection was previously resolved as sticky-approved, but after this change it resumes as rejected for that request, so old durable snapshots can change authorization behavior. Please thread the same per_call_overrides_sticky flag through hosted MCP approval restoration as well.
AGENTS.md reference: AGENTS.md:L168-L168
Useful? React with 👍 / 👎.
|
Thanks for reporting the issue. #4447 resolved it and its commit includes you as a co-author. Thanks again for your time and efforts here. |
Summary
Sticky and per-call approval decisions share the same two fields on
_ApprovalRecordbut use different shapes:always_approveandalways_rejectstore a bool, while a per-call decision stores call IDs in a list. Once a sticky decision existed, a later per-call decision on the same tool was lost.Rejecting one call after
always_approvewas ignored on read.get_approval_statusreturned atapproval_entry.approved is Truebefore reachingrejected_ids, so the rejection and its message were stored but never read, and the tool ran. In the other direction,always_rejectleaves the boolFalseinapproved, so the writer'sisinstance(target, list)guard was false and a laterapprove_toolwas dropped before it reached state.Rejecting a single call from the interruption list after approving a tool for the session is a normal human in the loop flow, so this reads as a safety issue rather than a cosmetic one.
The reader now checks a decision recorded for the exact call before falling back to the sticky value, and the writer starts a list when the field still holds a bool. Sticky behaviour for every other call is unchanged, and
always_approvefollowed byalways_rejectstill resolves toFalse.Test plan
Added
test_run_context_per_call_decision_overrides_sticky_decision, which covers both directions and asserts that other calls keep following the sticky decision. It fails on main and passes here..agents/skills/code-change-verification/scripts/run.shpasses: format, lint, typecheck and the full test suite.Issue number
Closes #4429
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR