review: Codex audit E2 authorization commits - #2
TheBayoumi wants to merge 5 commits into
Conversation
|
@codex review Review exactly the two commits in this PR:
Review focus:
Report only actionable P0/P1/P2 correctness, security, race, durability, or regression findings. Ignore unrelated existing PR mirror29#164 blockers unless either of these two commits worsens them. |
|
Review base: f18216e 🤖 DeepSeek V4 Pro PR Review |
![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: e1d10d9623
ℹ️ 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".
| toolName, | ||
| approvalInput, | ||
| reuseAfterConsume: toolName === "evolver.run_evolution", | ||
| reuseAfterConsume: durableEvolutionApproval, |
There was a problem hiding this comment.
For evolver.run_event_campaign, this generalization sets reuseAfterConsume to true, so the existing 24-hour consumed-operation cache and ledger return the approved operation ID on every matching call rather than permitting one compensation retry within two minutes. After a lost or failed /start response, the same owner/session/input can therefore invoke the tool repeatedly for a day without another decision; every invocation mints a fresh Ed25519 grant and replays the campaign-creation path. Backend idempotency prevents a second campaign for that operation ID, but it does not enforce the required authorization/retry bound, so E2 needs a separate two-minute, single-retry recovery policy.
AGENTS.md reference: AGENTS.md:L142-L145
Useful? React with 👍 / 👎.
|
@codex review Please re-review the complete three-commit contribution against base New head: The prior Codex P1 was addressed by making E2 recovery a bounded one-shot policy:
Please verify specifically:
Report only actionable P0/P1/P2 findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2196c5ded
ℹ️ 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 DURABLE_EVOLUTION_APPROVAL_TOOLS = new Set([ | ||
| "evolver.run_evolution", | ||
| "evolver.run_event_campaign", |
There was a problem hiding this comment.
With DATABASE_URL configured and the repository migrated through 0042_evolution_approval_operations.py, this newly durable E2 path attempts to insert tool_name='evolver.run_event_campaign', but the table still has CHECK (tool_name='evolver.run_evolution') at line 55 and no later migration alters it. The insert therefore fails, consumeApproved returns no operation ID, and every approved E2 launch is sent back through approval instead of executing; update the schema as part of this contribution.
AGENTS.md reference: AGENTS.md:L142-L145
Useful? React with 👍 / 👎.
| const persisted = await this.persistence?.rememberEvolutionOperation({ | ||
| ...scope, | ||
| operationId: record.requestId, | ||
| retentionMs, | ||
| }); |
There was a problem hiding this comment.
When two matching calls arrive concurrently immediately after approval, both can read the same approved record before either reaches this await, so both persist/cache the entitlement and both return the operation ID as initial executions. The durable row or overwritten in-memory one-shot entry then remains available for a third invocation, yielding an initial call plus two retries; this is fresh evidence in the new head that the prior one-retry issue remains under initial-consumption concurrency. Remove or transition the approved record atomically before the asynchronous persistence step, restoring it only on failure.
AGENTS.md reference: AGENTS.md:L142-L145
Useful? React with 👍 / 👎.
|
@codex review Please perform a final review of the complete contribution against base Final head: All prior Codex P1 findings have been addressed:
Full fork CI is green on this exact head: 54 files / 551 orchestration tests, migration apply/round-trip path, Paper/Evolver regressions, web/dashboard, consistency, agent eval, and self-host smoke. Please verify the previous P1s are closed and report only actionable P0/P1/P2 correctness, security, race, durability, migration, or regression findings. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
1 participant
Review-only Draft PR used for Codex review of the stacked E2 authorization contribution.
Do not merge. Review complete.
Base:
codex/evolution-task27atb75928d57fbeb7c978ccd33b84812921ca65408aFinal contribution head:
e1ec52ec48c3c6889c7faac0b8393bf1ec06f4bbReal upstream contribution: mirror29#167
Codex review history:
evolver.run_event_campaignand an initial-consumption concurrency race;e1ec52ec48: "Didn't find any major issues. Nice work!"Final fork CI on the exact head: PASS
Closed without merge.