Promotion sets + grace agent + lots of redesign#62
Conversation
…; beginning of the `grace agent` command; documentation updates; first-run scripts; much more tinkering.
![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: 9d7d2b79cf
ℹ️ 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".
| SessionId = | ||
| (if String.IsNullOrWhiteSpace parameters.OperationId then | ||
| Guid.NewGuid().ToString("N") | ||
| else | ||
| parameters.OperationId.Trim()) |
There was a problem hiding this comment.
The new session identifier is built from parameters.OperationId, but that field is also used as the idempotency token (replayKey) and can be reused by callers across independent starts. Because activeAgentSessionsBySessionId is keyed only by this SessionId, a later start using the same operation id can overwrite the earlier mapping, which breaks /agent/session/status and /agent/session/stop lookups by session id for the original active session. Session identity should be generated independently from the idempotency token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex is your recommendation to use Guid.NewGuid() every time?
| else | ||
| let mutable stepId = Guid.Empty | ||
|
|
||
| if not <| Guid.TryParse(parameters.StepId, &stepId) then |
There was a problem hiding this comment.
ResolveConflicts validates StepId with Guid.TryParse, which accepts 00000000-0000-0000-0000-000000000000; that value then flows to the actor. In PromotionSetActor.RecomputeSteps, manual decisions are only applied when the supplied step id equals a real step id, so an empty guid is silently ignored and the request can appear to succeed without applying any of the provided resolutions. Treat Guid.Empty as InvalidPromotionSetStepId at the API boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Fixed
ResolveConflictsstep ID validation so the API now rejects both unparseable IDs andGuid.Empty, returningInvalidPromotionSetStepIdbefore calling the actor. This closes the gap where00000000-0000-0000-0000-000000000000could be accepted and silently ignored downstream. src/Grace.Server/PromotionSet.Server.fsL343-L350 - Committed the change on the current branch (
9dcd143) and created a follow-up PR entry via themake_prtool.
Testing
- ✅
curl -s https://learn.microsoft.com/en-us/dotnet/api/system.guid.tryparse?view=net-9.0 | head -n 20 ⚠️ dotnet build src/Grace.Server/Grace.Server.fsproj -c Release(environment limitation:dotnetCLI is not installed,command not found)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9a01b11ff
ℹ️ 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".
| (if String.IsNullOrWhiteSpace parameters.OperationId then | ||
| Guid.NewGuid().ToString("N") | ||
| else | ||
| parameters.OperationId.Trim()) |
There was a problem hiding this comment.
OperationId is defined as a caller-supplied idempotency token, but this code reuses it as SessionId, which lets different sessions collide on the global activeAgentSessionsBySessionId index; once that happens, tryResolveActiveSession prefers the collided SessionId path and can return the wrong session (or fail to find the real one) for stop/status calls. This is especially likely when callers reuse operation IDs across retries or agents, so each new session should get its own unique ID (for example Guid.NewGuid()), while OperationId remains only for replay caching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated agent session creation to always generate a new
SessionIdviaGuid.NewGuid().ToString("N"), instead of reusing caller-suppliedOperationId. This decouples session identity from idempotency replay keys and prevents session-id collisions in the active session index. src/Grace.Server/Startup.Server.fsL431-L445 - Committed the fix on the current branch (
b3b002c) and created a follow-up PR entry withmake_pr.
Testing
- ✅
curl -s https://learn.microsoft.com/en-us/dotnet/api/system.guid.newguid?view=net-9.0 | head -n 20 ⚠️ dotnet build src/Grace.Server/Grace.Server.fsproj -c Release(environment limitation:dotnetCLI is not installed,command not found)
There was a problem hiding this comment.
Why keep an index if it's always new?
2 participants
This PR is a massive redesign of the promotion-set construct, getting rid of unused CLI command structures, and starting to add features that directly help reviewers, including the beginning of
grace agentimplementation.