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: 5ef6c60ce7
ℹ️ 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".
| // Resolve branch for new structured storage | ||
| let branch: string | undefined; | ||
| try { | ||
| branch = (await gitService.getCurrentBranch()).trim(); |
There was a problem hiding this comment.
The detached-HEAD fallback never triggers here because git rev-parse --abbrev-ref HEAD returns the literal string HEAD instead of throwing, so branch stays truthy and the command writes/sends captures as branch HEAD instead of using legacy SHA storage. In detached checkouts (common in CI or bisect/rebase flows), this collapses unrelated captures into .kody/pr/HEAD.md and misattributes API data; normalize HEAD (and empty results) to undefined before the branch-path branch.
Useful? React with 👍 / 👎.
src/commands/memory/hooks.ts
Outdated
| if (line.trim() === 'fi') { | ||
| inKodusBlock = false; |
There was a problem hiding this comment.
The uninstall logic exits inKodusBlock at the first fi, but the injected script from installMergeHook contains two if blocks, so disabling after a normal install leaves the trailing promotion block in .git/hooks/post-merge. Because the marker line is removed in that first pass, subsequent kodus decisions disable runs cannot find and clean this leftover block, leaving a partially modified hook indefinitely.
Useful? React with 👍 / 👎.
|
The PR introduces checks for new environment variables: |
|
This pull request does not appear to reference any issues. Please update the PR description to include a reference like |
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
docs/api-contract-memory-captures.md
Outdated
| - Classificar cada decisão por tipo (`architectural_decision`, `convention`, `tradeoff`, etc.) | ||
| - Associar ao `orgRepo` e `branch` | ||
| 3. **Retornar rapidamente** — o CLI não espera, mas o timeout padrão do `request` é 20min (herança do request genérico). Idealmente responder em < 1s. | ||
| 4. **Idempotência** — considerar usar `sha` + `sessionId` + `capturedAt` como chave de dedup |
There was a problem hiding this comment.
Data loss due to flawed idempotency key. The suggested idempotency key on line 139 is sha + sessionId + capturedAt. According to the contract itself, sha can be null (line 37) and sessionId is optional (line 51). If two different captures are sent without these fields at the exact same time, their idempotency keys will collide. The backend will incorrectly discard the second capture as a duplicate, leading to silent data loss. A more robust key should be used, incorporating required fields like branch.
4. **Idempotência** — A chave de dedup `sha` + `sessionId` + `capturedAt` é frágil e pode causar perda de dados, pois `sha` e `sessionId` são opcionais/nulos. Considere uma chave mais robusta que inclua campos obrigatórios, como `branch`.Prompt for LLM
File docs/api-contract-memory-captures.md:
Line 139:
I am reviewing an API contract document written in Markdown. The document specifies a new endpoint for submitting 'memory captures'. In the 'backend requirements' section, it suggests a strategy for idempotency to avoid processing duplicate captures. The suggested idempotency key is a combination of three fields from the request body: `sha`, `sessionId`, and `capturedAt`. However, earlier in the same document, the contract defines the `sha` field as nullable (`string | null`) and the `sessionId` field as optional (`string?`). Please identify the flaw in this idempotency strategy and explain the negative consequences, such as data loss. Also, suggest a more robust key using other fields defined in the contract, keeping in mind that the `branch` field is a required string.
Suggested Code:
4. **Idempotência** — A chave de dedup `sha` + `sessionId` + `capturedAt` é frágil e pode causar perda de dados, pois `sha` e `sessionId` são opcionais/nulos. Considere uma chave mais robusta que inclua campos obrigatórios, como `branch`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| vi.mock('../../services/git.service.js', () => ({ | ||
| gitService: { | ||
| isGitRepository: vi.fn().mockResolvedValue(true), | ||
| getGitRoot: vi.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| let tmpDir: string; | ||
|
|
||
| beforeEach(async () => { | ||
| tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-merge-hook-test-')); | ||
| const hooksDir = path.join(tmpDir, '.git', 'hooks'); | ||
| await fs.mkdir(hooksDir, { recursive: true }); | ||
| }); |
There was a problem hiding this comment.
The mock for gitService.getGitRoot is configured statically as vi.fn(), which returns a promise that resolves to undefined. The function under test, installMergeHook, requires the git repository's root path to operate. When it calls the mocked getGitRoot function, it receives undefined, and any subsequent path-joining or file system operations on this value will throw a TypeError, causing the test to crash. Mocks that depend on dynamic state generated within beforeEach (like the tmpDir path) must be configured inside beforeEach after that state has been initialized.
import { gitService } from '../../services/git.service.js';
vi.mock('../../services/git.service.js');
let tmpDir: string;
beforeEach(async () => {
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-merge-hook-test-'));
const hooksDir = path.join(tmpDir, '.git', 'hooks');
await fs.mkdir(hooksDir, { recursive: true });
(gitService.isGitRepository as vi.Mock).mockResolvedValue(true);
(gitService.getGitRoot as vi.Mock).mockResolvedValue(tmpDir);
});Prompt for LLM
File src/commands/__tests__/install-merge-hook.test.ts:
Line 7 to 20:
I am writing a test file using vitest for a function that installs a git hook. I've mocked the `gitService` dependency, but I think I've done it incorrectly. The mock for `getGitRoot` is defined at the top level of the file, but its return value needs to be a temporary directory path that is only created inside the `beforeEach` hook for each test. This causes the mock to return `undefined` and the test crashes with a `TypeError`. What is the correct way to structure my mocks in vitest when the mock's return value depends on state that is set up for each individual test?
Suggested Code:
import { gitService } from '../../services/git.service.js';
vi.mock('../../services/git.service.js');
let tmpDir: string;
beforeEach(async () => {
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-merge-hook-test-'));
const hooksDir = path.join(tmpDir, '.git', 'hooks');
await fs.mkdir(hooksDir, { recursive: true });
(gitService.isGitRepository as vi.Mock).mockResolvedValue(true);
(gitService.getGitRoot as vi.Mock).mockResolvedValue(tmpDir);
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const gitRoot = (await gitService.getGitRoot()).trim(); | ||
|
|
||
| const claudeResult = await removeClaudeCompatibleHooks(gitRoot); | ||
| const codexResult = await removeCodexNotify(resolveCodexConfigPath()); |
There was a problem hiding this comment.
Path resolution for the Codex configuration is based on the current working directory, not the git repository root. The resolveCodexConfigPath function is called without arguments, causing it to use process.cwd(). However, other path-dependent functions in this file correctly receive the gitRoot variable. If this command is executed from a subdirectory of the repository, resolveCodexConfigPath will fail to locate the configuration file at the repository root, causing the hook removal to fail silently. The gitRoot variable should be passed to resolveCodexConfigPath to ensure consistent and correct path resolution.
const codexResult = await removeCodexNotify(resolveCodexConfigPath(gitRoot));Prompt for LLM
File src/commands/memory/disable.ts:
Line 20:
I have a TypeScript function in a command-line tool that operates on a git repository. The function correctly determines the repository's root directory and stores it in a `gitRoot` variable. It then calls three separate helper functions to remove different git hooks. Two of these helpers are correctly passed the `gitRoot` variable to operate on the correct paths. However, a third helper, `resolveCodexConfigPath()`, is called without any arguments. This causes it to resolve a configuration file path relative to the current working directory, which can be different from the repository root if the user runs the command from a subdirectory. This leads to a silent failure where one of the hooks is not removed. How should I modify the problematic line to ensure `resolveCodexConfigPath` uses the repository's root directory for its path calculations, just like the other helper functions?
Suggested Code:
const codexResult = await removeCodexNotify(resolveCodexConfigPath(gitRoot));
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| force?: boolean; | ||
| } | ||
|
|
||
| export async function enableAction(options: EnableOptions): Promise<void> { |
There was a problem hiding this comment.
Unhandled promise rejections from multiple asynchronous operations will crash the process. While the parseAgents function call is correctly wrapped in a try-catch block, other awaited calls such as gitService.isGitRepository, installClaudeCompatibleHooks, detectModules, and fs.writeFile are not. If any of these operations fail due to environmental issues (e.g., file system permissions, git not being installed), they will throw an exception. This results in an unhandled promise rejection, which crashes the CLI tool with a stack trace instead of displaying a user-friendly error message and exiting gracefully.
export async function enableAction(options: EnableOptions): Promise<void> {
try {
const isRepo = await gitService.isGitRepository();
if (!isRepo) {
console.error(chalk.red('Error: Not a git repository.'));
process.exit(1);
}
const gitRoot = (await gitService.getGitRoot()).trim();
let agents: Set<string>;
try {
agents = parseAgents(options.agents ?? 'claude,cursor,codex');
} catch (error) {
console.error(chalk.red((error as Error).message));
process.exit(1);
return;
}
const installClaudeCompat = agents.has('claude') || agents.has('cursor');
const installCodex = agents.has('codex');
// 1. Claude Code / Cursor hooks
let claudeStatus = 'skipped';
if (installClaudeCompat) {
const result = await installClaudeCompatibleHooks(gitRoot);
claudeStatus = result.changed ? 'installed' : 'already configured';
}
// 2. Codex notify
let codexStatus = 'skipped';
if (installCodex) {
const codexConfigPath = resolveCodexConfigPath(options.codexConfig);
const result = await installCodexNotify(codexConfigPath);
if (result.changed) {
codexStatus = 'installed';
} else if (result.skipped) {
codexStatus = 'skipped (existing notify entry)';
} else {
codexStatus = 'already configured';
}
}
// 3. Post-merge hook (always)
const mergeResult = await installMergeHook(gitRoot);
const mergeStatus = mergeResult.alreadyInstalled
? 'already configured'
: 'installed';
// 4. Init modules.yml
const configPath = path.join(gitRoot, '.kody', 'modules.yml');
let modulesStatus: string;
let modulesExist = false;
try {
await fs.access(configPath);
modulesExist = true;
} catch {
// doesn't exist
}
if (modulesExist && !options.force) {
modulesStatus = 'already exists';
} else {
const srcPath = path.join(gitRoot, 'src');
const modules = await detectModules(srcPath);
const config: ModulesYml = { version: 1, modules };
const yamlContent = stringifyYaml(config);
await fs.mkdir(path.dirname(configPath), { recursive: true });
await fs.writeFile(configPath, yamlContent, 'utf-8');
modulesStatus =
modules.length > 0
? `created (${modules.length} module${
modules.length === 1 ? '' : 's'
} detected)`
: 'created (no modules detected)';
}
// Summary
console.log(chalk.green('\u2713 Decisions enabled for this repository.'));
console.log(` Claude Code / Cursor hooks: ${claudeStatus}`);
console.log(` Codex notify: ${codexStatus}`);
console.log(` Post-merge hook: ${mergeStatus}`);
console.log(` Module config: ${modulesStatus}`);
} catch (error) {
console.error(chalk.red('An unexpected error occurred during the enable action:'));
console.error(error);
process.exit(1);
}
}Prompt for LLM
File src/commands/memory/enable.ts:
Line 22:
The provided TypeScript code defines an asynchronous function `enableAction` for a command-line tool. This function performs several asynchronous operations, including interacting with git and the file system. One of these operations, `parseAgents`, is correctly wrapped in a try-catch block to handle potential errors gracefully. However, many other `await` calls, such as `gitService.isGitRepository()`, `installClaudeCompatibleHooks()`, `detectModules()`, and `fs.writeFile()`, are not inside a try-catch block. If any of these unhandled async operations were to fail (for example, due to a file system permission error or a problem with the git environment), it would result in an unhandled promise rejection. In Node.js, an unhandled promise rejection will terminate the process, causing the command-line tool to crash and display a raw stack trace to the user instead of a clean, understandable error message. Please refactor the `enableAction` function to include comprehensive error handling that catches any potential exceptions from its async operations, logs a user-friendly error message, and exits the process cleanly.
Suggested Code:
export async function enableAction(options: EnableOptions): Promise<void> {
try {
const isRepo = await gitService.isGitRepository();
if (!isRepo) {
console.error(chalk.red('Error: Not a git repository.'));
process.exit(1);
}
const gitRoot = (await gitService.getGitRoot()).trim();
let agents: Set<string>;
try {
agents = parseAgents(options.agents ?? 'claude,cursor,codex');
} catch (error) {
console.error(chalk.red((error as Error).message));
process.exit(1);
return;
}
const installClaudeCompat = agents.has('claude') || agents.has('cursor');
const installCodex = agents.has('codex');
// 1. Claude Code / Cursor hooks
let claudeStatus = 'skipped';
if (installClaudeCompat) {
const result = await installClaudeCompatibleHooks(gitRoot);
claudeStatus = result.changed ? 'installed' : 'already configured';
}
// 2. Codex notify
let codexStatus = 'skipped';
if (installCodex) {
const codexConfigPath = resolveCodexConfigPath(options.codexConfig);
const result = await installCodexNotify(codexConfigPath);
if (result.changed) {
codexStatus = 'installed';
} else if (result.skipped) {
codexStatus = 'skipped (existing notify entry)';
} else {
codexStatus = 'already configured';
}
}
// 3. Post-merge hook (always)
const mergeResult = await installMergeHook(gitRoot);
const mergeStatus = mergeResult.alreadyInstalled
? 'already configured'
: 'installed';
// 4. Init modules.yml
const configPath = path.join(gitRoot, '.kody', 'modules.yml');
let modulesStatus: string;
let modulesExist = false;
try {
await fs.access(configPath);
modulesExist = true;
} catch {
// doesn't exist
}
if (modulesExist && !options.force) {
modulesStatus = 'already exists';
} else {
const srcPath = path.join(gitRoot, 'src');
const modules = await detectModules(srcPath);
const config: ModulesYml = { version: 1, modules };
const yamlContent = stringifyYaml(config);
await fs.mkdir(path.dirname(configPath), { recursive: true });
await fs.writeFile(configPath, yamlContent, 'utf-8');
modulesStatus =
modules.length > 0
? `created (${modules.length} module${
modules.length === 1 ? '' : 's'
} detected)`
: 'created (no modules detected)';
}
// Summary
console.log(chalk.green('\u2713 Decisions enabled for this repository.'));
console.log(` Claude Code / Cursor hooks: ${claudeStatus}`);
console.log(` Codex notify: ${codexStatus}`);
console.log(` Post-merge hook: ${mergeStatus}`);
console.log(` Module config: ${modulesStatus}`);
} catch (error) {
console.error(chalk.red('An unexpected error occurred during the enable action:'));
console.error(error);
process.exit(1);
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const decisionCount = (prMemory.content.match(/^### \[\w+\]/gm) || []).length; | ||
| const captureCount = (prMemory.content.match(/^### \d{4}-\d{2}-\d{2}T/gm) || []).length; |
There was a problem hiding this comment.
Null pointer dereference. The code checks for the existence of prMemory.meta on line 29 but then unconditionally accesses prMemory.content on lines 38 and 39. If the memoryService.readPrMemory function returns an object that contains a meta property but is missing the content property, the application will crash with a TypeError when attempting to call .match on undefined.
| const decisionCount = (prMemory.content.match(/^### \[\w+\]/gm) || []).length; | |
| const captureCount = (prMemory.content.match(/^### \d{4}-\d{2}-\d{2}T/gm) || []).length; | |
| const decisionCount = (prMemory.content?.match(/^### \[\w+\]/gm) || []).length; | |
| const captureCount = (prMemory.content?.match(/^### \d{4}-\d{2}-\d{2}T/gm) || []).length; |
Warning
This is an experimental feature that generates committable changes. Review the diff before applying. Results may be incorrect.
Prompt for LLM
File src/commands/memory/status.ts:
Line 38 to 39:
I have a TypeScript function that processes an object called `prMemory`. The code checks if `prMemory.meta` exists, and if it does, it proceeds to access `prMemory.content`. However, there is no check to ensure `prMemory.content` also exists. This can lead to a `TypeError` if `prMemory` has a `meta` property but no `content` property, as the code will try to call the `.match()` method on `undefined`. Please refactor the code to prevent this crash, for example by using optional chaining.
Suggested Code:
// Count decisions in content
const decisionCount = (prMemory.content?.match(/^### \[\w+\]/gm) || []).length;
const captureCount = (prMemory.content?.match(/^### \d{4}-\d{2}-\d{2}T/gm) || []).length;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return { promoted: 0, modules: [] }; | ||
| } | ||
|
|
||
| let promoted = 0; |
There was a problem hiding this comment.
The current implementation in promoteToModuleMemory uses nested loops to associate decisions with modules, which is inefficient. The outer loop iterates through targetModuleIds, and for each module, the inner loop iterates through all decisions. This structure leads to poor performance (O(n*m) time complexity) due to repeated lookups with .find() and .includes() inside the loops.
To optimize this, the logic should be inverted. First, process the decisions list once to group them by moduleId using a Map. Then, iterate through the targetModuleIds and use the map for a highly efficient O(1) lookup of the relevant decisions for each module. This change will significantly improve performance by avoiding the costly nested loop.
Kody Rule violation: Prefer Map for lookups inside loops
const modulesMap = new Map(config.modules.map((m) => [m.id, m]));
const decisionsByModule = new Map<string, typeof decisions>();
// Group decisions by module ID
for (const decision of decisions) {
const decisionModules = matchFiles(decision.files, config.modules);
for (const moduleId of decisionModules) {
if (!decisionsByModule.has(moduleId)) {
decisionsByModule.set(moduleId, []);
}
decisionsByModule.get(moduleId)!.push(decision);
}
}
let promoted = 0;
for (const moduleId of targetModuleIds) {
const mod = modulesMap.get(moduleId);
const decisionsForModule = decisionsByModule.get(moduleId);
if (!mod || !decisionsForModule) {
continue;
}
const memoryFilePath = path.join(repoRoot, mod.memoryFile);
await fs.mkdir(path.dirname(memoryFilePath), { recursive: true });
const header = await this.ensureModuleMemoryHeader(memoryFilePath, mod.name);
let content = header;
for (const decision of decisionsForModule) {
const decisionFiles = decision.files;
content += `\n### ${decision.title}\n`;
content += `- **Type:** ${decision.type}\n`;
content += `- **Rationale:** ${decision.rationale}\n`;
if (decisionFiles.length > 0) {
content += `- **Files:** ${decisionFiles.join(', ')}\n`;
}
content += `- **Source:** ${branch} / ${decision.source}\n`;
promoted++;
}
await fs.writeFile(memoryFilePath, content, 'utf-8');
}
return { promoted, modules: targetModuleIds };Prompt for LLM
File src/services/memory.service.ts:
Line 211:
I need to refactor a piece of my TypeScript code for performance. The function `promoteToModuleMemory` currently iterates through each target module, and for each module, it iterates through all available decisions to find matches. This nested loop structure is inefficient.
My coding standards require avoiding inefficient lookups like `.find()` or `.includes()` inside loops, as this can lead to quadratic time complexity. The current code violates this with both a `.find()` call in the outer loop and an `.includes()` call in the inner loop.
Please refactor this function to be more performant. The ideal solution would iterate through the decisions once, grouping them by the modules they belong to (using a Map), and then iterate through the target modules to write the files using the pre-grouped decisions. This will eliminate the nested loop and improve efficiency.
Suggested Code:
const modulesMap = new Map(config.modules.map((m) => [m.id, m]));
const decisionsByModule = new Map<string, typeof decisions>();
// Group decisions by module ID
for (const decision of decisions) {
const decisionModules = matchFiles(decision.files, config.modules);
for (const moduleId of decisionModules) {
if (!decisionsByModule.has(moduleId)) {
decisionsByModule.set(moduleId, []);
}
decisionsByModule.get(moduleId)!.push(decision);
}
}
let promoted = 0;
for (const moduleId of targetModuleIds) {
const mod = modulesMap.get(moduleId);
const decisionsForModule = decisionsByModule.get(moduleId);
if (!mod || !decisionsForModule) {
continue;
}
const memoryFilePath = path.join(repoRoot, mod.memoryFile);
await fs.mkdir(path.dirname(memoryFilePath), { recursive: true });
const header = await this.ensureModuleMemoryHeader(memoryFilePath, mod.name);
let content = header;
for (const decision of decisionsForModule) {
const decisionFiles = decision.files;
content += `\n### ${decision.title}\n`;
content += `- **Type:** ${decision.type}\n`;
content += `- **Rationale:** ${decision.rationale}\n`;
if (decisionFiles.length > 0) {
content += `- **Files:** ${decisionFiles.join(', ')}\n`;
}
content += `- **Source:** ${branch} / ${decision.source}\n`;
promoted++;
}
await fs.writeFile(memoryFilePath, content, 'utf-8');
}
return { promoted, modules: targetModuleIds };
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
The pull request description does not contain a reference to a related issue. Please update the description to include a closing statement like 'Closes #123' or 'Fixes #123' to ensure issues are automatically closed and work is properly tracked. Kody Rule violation: Ensure PR closes referenced issues |
1 participant
Introduces a new "Decision Memory" feature to capture and manage coding session decisions made by AI agents.
Key Changes:
kodus decisions captureto record agent interactions (prompts, assistant messages, modified files, tool uses) into repository-local markdown files (.kody/pr/<branch-name>.md)./cli/memory/captures) for further processing (e.g., LLM classification), using a fire-and-forget mechanism to ensure non-blocking operation.kodus decisions enableto install necessary hooks for Claude Code, Cursor, and Codex agents, and to initialize amodules.ymlconfiguration file. Thismodules.ymldefines project modules and their associated file paths for decision promotion.post-mergehook to automatically trigger decision promotion when branches are merged.kodus decisions disableto remove all installed hooks while preserving local memory data.kodus decisions promoteto move decisions from branch-specific PR memory to module-specific memory files (.kody/memory/<module-id>.md), based on file changes and themodules.ymlconfiguration.kodus decisions statusto display the current branch's PR memory status and module configuration.kodus decisions showto view PR memory for a branch or module memory for a specific module.transcriptParserServiceto standardize raw agent payloads into a consistent signal format.module-matcherutility to load module configurations and match files to defined modules using glob patterns.currentdevice count if available.