fix(decisions): use stop event for codex notify#10
Conversation
This comment has been minimized.
This comment has been minimized.
![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: f9131add15
ℹ️ 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".
|
|
||
| export const CODEX_NOTIFY_LINE = | ||
| 'notify = ["kodus", "decisions", "capture", "--agent", "codex", "--event", "agent-turn-complete"]'; | ||
| 'notify = ["kodus", "decisions", "capture", "--agent", "codex", "--event", "stop"]'; |
There was a problem hiding this comment.
Changing CODEX_NOTIFY_LINE to --event stop breaks upgrade behavior for users who already have the old --event agent-turn-complete line in ~/.codex/config.toml: installCodexNotify and removeCodexNotify both rely on exact-string matching via this constant, so after this change those existing installs are neither recognized as configured nor removable, and enable now skips updating them because the generic notify regex detects an existing entry. In practice, an upgraded user who reruns kodus decisions enable/disable can get stuck with the legacy notify command and no automatic migration path.
Useful? React with 👍 / 👎.
|
The pull request description is empty. To improve tracking and provide context, please add a reference to the issue being addressed, for example, 'Closes #123' or 'Fixes #456'. Kody Rule violation: Ensure PR closes referenced issues |
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
This pull request does not seem to reference any issue. Please update the description to include a closing keyword like 'Closes #issue_number' or 'Fixes #issue_number' to automatically link and close the relevant issue upon merging. Kody Rule violation: Ensure PR closes referenced issues |
| } | ||
|
|
||
| if (content.includes(CODEX_NOTIFY_LINE_LEGACY)) { | ||
| const nextContent = content.replace(CODEX_NOTIFY_LINE_LEGACY, CODEX_NOTIFY_LINE); |
There was a problem hiding this comment.
State corruption due to partial replacement in configuration file. The function installCodexNotify uses String.prototype.replace() to update a legacy configuration string. When the pattern is a string, replace() only affects the first occurrence. If a user's configuration file contains multiple instances of the legacy notification line (CODEX_NOTIFY_LINE_LEGACY), only the first one will be updated to the new version. All subsequent instances will remain, leaving the configuration file in a corrupted state with both old and new notification commands. This will cause unexpected behavior, such as duplicate command execution or configuration parsing errors.
const nextContent = content.replaceAll(CODEX_NOTIFY_LINE_LEGACY, CODEX_NOTIFY_LINE);Prompt for LLM
File src/commands/memory/hooks.ts:
Line 227:
I have a TypeScript function that reads a configuration file as a string, finds a legacy configuration line, and replaces it with a new one. The current implementation uses `String.prototype.replace()`. The problem is that `replace()` with a string argument only replaces the first match. If the legacy configuration line appears multiple times in the file, this code will only update the first one, leaving the file in a corrupted state with both old and new configuration lines. Please rewrite the line of code responsible for the replacement to ensure that all occurrences of the legacy configuration line are replaced with the new one.
Suggested Code:
const nextContent = content.replaceAll(CODEX_NOTIFY_LINE_LEGACY, CODEX_NOTIFY_LINE);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
1 participant
Updates the
codexagent's notification configuration to trigger decision capture on thestopevent instead of theagent-turn-completeevent. This ensures that thekodus decisions capturecommand is executed when thecodexagent fully stops. Integration tests have been updated to reflect this change in the expected configuration.