Skip to content

Feat/local memory provider - #42

Merged
Abdulmuiz44 merged 3 commits into
mainfrom
feat/local-memory-provider
Jun 13, 2026
Merged

Feat/local memory provider#42
Abdulmuiz44 merged 3 commits into
mainfrom
feat/local-memory-provider

Conversation

@Abdulmuiz44

Copy link
Copy Markdown
Collaborator

No description provided.

@Abdulmuiz44
Abdulmuiz44 merged commit aecd535 into main Jun 13, 2026
1 check passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cf975c6050

ℹ️ 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".

Comment on lines +185 to +186
const result = run(['understand'], { cwd: repo });
assertIncludes(result.stdout, 'Codra Understand', 'understand output');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove unsupported understand smoke test

This new smoke test calls codra understand, but the reviewed tree has no understand subcommand in crates/codra-cli/src/main.rs and git grep only finds this test expectation. The wrapper therefore prints the normal help for an unknown command, so npm test fails at the Codra Understand assertion before the release workflow can package the CLI.

Useful? React with 👍 / 👎.

Comment on lines +203 to +207
const dryRun = spawnSync('npm', ['pack', '--dry-run'], {
cwd: packageRoot,
encoding: 'utf8',
env: process.env,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve release platform env for pack smoke

In the release workflow's Test npm wrapper step, the environment only sets CODRA_USE_ARTIFACTS, while the default matrix intentionally omits darwin-x64. This new npm pack --dry-run invokes prepack; without CODRA_EXPECT_PLATFORMS/CODRA_ALLOW_PARTIAL_BINARIES, build-platform-binaries.js falls back to requiring all five artifacts, including the unbuilt codra-darwin-x64, so the default release workflow fails even after artifacts were packaged successfully in the previous step.

Useful? React with 👍 / 👎.

Comment on lines +26 to +27
crossterm = "0.28"
ratatui = "0.29"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Commit Cargo.lock for new CLI dependencies

These dependencies were added without updating Cargo.lock; the reviewed lockfile still has no crossterm or ratatui package entries. Since cargo build --help defines --locked as asserting that Cargo.lock remains unchanged, any locked/reproducible build of this repo will fail before compilation until the regenerated lockfile is committed with the manifest change.

Useful? React with 👍 / 👎.

}

const tarball = path.join(packageRoot, match[1]);
const install = spawnSync('npm', ['install', '-g', tarball], {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid installing test tarballs globally

When npm test runs on a developer machine or shared CI image, this npm install -g uses the real global prefix (npm install --help lists -g|--global) and never uninstalls it. That can fail if another package already owns the codra shim or leave this tarball as the user's global codra, so use a temporary --prefix/sandboxed install for the version check instead.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant