feat: add wagl forget command [DO NOT MERGE] by GoZumie · Pull Request #89 · BigInformatics/wagl

GoZumie · 2026-03-16T23:30:40Z

Summary

Implements wagl forget <id> command (issue #87) — deletes a memory from the DB by its ID.

Changes

wagl-db: Added delete_item(id) method to MemoryDb
wagl-cli: Added forget subcommand with:
- wagl forget <id> — dry-run preview by default
- wagl forget <id> --confirm — actually deletes
- wagl forget <id> --json — JSON output for scripting
wagl-server: Minor cleanup

Testing

cargo build passes clean

Closes #87

PR Review by Greptile

Greptile Summary

This PR adds a wagl forget <id> command that previews (dry-run by default) and deletes a memory item plus its associated embeddings, edges, and vector entries. The CLI plumbing is well-structured and follows existing patterns, but three correctness issues were found in the new handler.

Key issues:

False deletion confirmation (logic bug): The return value of db.delete(&id) (number of rows affected) is discarded. If the item is concurrently deleted between the db.get check and the delete call, the handler still prints "deleted": true with exit code 0, which is incorrect for a destructive command.
Runtime panic on non-ASCII text (logic bug): &item.text[..std::cmp::min(120, item.text.len())] uses byte offsets to slice a UTF-8 string. Any stored text with multibyte characters (accents, CJK, emoji) will cause a panic at this point. A char-aware approach (chars().take(120).collect()) should be used instead. (This pattern pre-exists in the codebase at several other sites and should ideally be fixed globally.)
Inconsistent exit code on "not found" (logic bug): When an ID is not found and --json is active, the command exits with code 2 (NOT_FOUND). Without --json, it exits 0 with a {"found": false} JSON body and no stderr message — misleading for non-JSON callers of a destructive command.
PR description accuracy: The description documents wagl forget <id> --json, but --json is a global flag; the correct invocation is wagl --json forget <id>.

Confidence Score: 2/5

Not safe to merge — contains a logic bug that falsely confirms deletion and a panic path for non-ASCII text.
The unchecked delete return value means a concurrent second invocation silently reports success without deleting anything; the byte-slice panic is a crash for any user storing non-ASCII content. Both issues require fixes before this command can be used reliably.
crates/cli/src/main.rs — the Forget handler (lines 4032–4078)

Important Files Changed

Filename	Overview
crates/cli/src/main.rs	Adds `Forget` subcommand and handler; has three notable issues: (1) `db.delete()` return value ignored — can falsely report `"deleted": true` on concurrent deletion, (2) byte-offset slicing on UTF-8 text can panic for non-ASCII content, (3) not-found path exits with code 0 in non-JSON mode vs. code 2 in JSON mode.

Sequence Diagram

sequenceDiagram
    actor User
    participant CLI as wagl CLI
    participant DB as MemoryDb

    User->>CLI: wagl forget <id> [--confirm]
    CLI->>DB: connect_auto() + init()
    CLI->>DB: get(id)
    DB-->>CLI: Option<MemoryItem>

    alt Item not found
        CLI-->>User: json_mode=true → exit 2 NOT_FOUND\njson_mode=false → {"found":false} exit 0
    else Item found — dry-run (no --confirm)
        CLI-->>User: {"mode":"dry-run","item":preview,"hint":"..."}
    else Item found — --confirm
        CLI->>DB: delete(id) [return value discarded ⚠️]
        DB->>DB: DELETE embeddings_meta (silent)
        DB->>DB: DELETE memory_edges (silent)
        DB->>DB: DELETE memory_item_vec (silent)
        DB->>DB: DELETE memory_items
        DB-->>CLI: u64 affected rows (ignored)
        CLI-->>User: {"deleted":true,"item":preview}
    end

_{Last reviewed commit: 2165a0e}

Greptile also left 3 inline comments on this PR.

Context used:

Rule used - AGENTS.md (source)

Implements the single-item deletion path from issue #87. Shows item details in a dry-run by default; pass --confirm to actually delete. Reuses the existing MemoryDb::delete() which cleans up embeddings, edges, and vector data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2165a0e9e8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-16T23:35:54Z

crates/cli/src/main.rs

+                    "salience": item.salience,
+                    "created_at": item.created_at,
+                    "tags": item.tags,
+                    "text_preview": &item.text[..std::cmp::min(120, item.text.len())]


Truncate preview text on char boundaries

The preview uses a byte slice (&item.text[..min(120, item.text.len())]), which can panic when the 120th byte falls inside a multi-byte UTF-8 character. Any memory containing non-ASCII text (e.g., emoji/CJK) around that cutoff will cause wagl forget <id> to crash before returning output or deleting, even with --confirm. Please build text_preview using a character-boundary-safe truncation method instead of byte indexing.

Useful? React with 👍 / 👎.

greptile-apps · 2026-03-16T23:36:26Z

crates/cli/src/main.rs

+                    db.delete(&id).await?;
+                    print_ok(json_mode, serde_json::json!({
+                        "deleted": true,
+                        "item": preview
+                    }))?;


Unchecked delete return value — can falsely confirm deletion

db.delete(&id) returns anyhow::Result<u64> where the u64 is the number of affected rows. The result is awaited and the error is propagated, but the affected-row count is silently discarded. If the item is deleted between the db.get check and this call (concurrent process / script), affected will be 0, yet the code still prints "deleted": true.

Because this is a destructive operation, the response should verify that a row was actually removed before claiming success:

Suggested change

db.delete(&id).await?;

print_ok(json_mode, serde_json::json!({

"deleted": true,

"item": preview

}))?;

let affected = db.delete(&id).await?;

if affected == 0 {

if json_mode {

return Err(anyhow::Error::new(CliError::NotFound(format!(

"memory item not found (concurrent deletion?): {}",

id

))));

} else {

print_ok(json_mode, serde_json::json!({

"deleted": false,

"id": id,

"hint": "item may have been deleted concurrently"

}))?;

return Ok(());

}

}

print_ok(json_mode, serde_json::json!({

"deleted": true,

"item": preview

}))?;

greptile-apps · 2026-03-16T23:36:27Z

crates/cli/src/main.rs

+                    "salience": item.salience,
+                    "created_at": item.created_at,
+                    "tags": item.tags,
+                    "text_preview": &item.text[..std::cmp::min(120, item.text.len())]


Byte-index slice on UTF-8 string will panic on multibyte characters

&item.text[..std::cmp::min(120, item.text.len())] slices the string at byte position 120. If item.text contains multibyte UTF-8 characters (e.g. accented letters, CJK, emoji), byte offset 120 may land in the middle of a code point, causing a runtime panic: "byte index N is not a char boundary".

The same pre-existing pattern appears at lines 1908, 3099–3100, 3121, 3159, and 3683, but this PR introduces a new instance of it.

Use character-aware slicing to avoid the panic:

Suggested change

"text_preview": &item.text[..std::cmp::min(120, item.text.len())]

"text_preview": item.text.chars().take(120).collect::<String>()

greptile-apps · 2026-03-16T23:36:28Z

crates/cli/src/main.rs

+                let Some(item) = item else {
+                    if json_mode {
+                        return Err(anyhow::Error::new(CliError::NotFound(format!(
+                            "memory item not found: {}",
+                            id
+                        ))));
+                    } else {
+                        print_ok(json_mode, serde_json::json!({
+                            "found": false,
+                            "id": id
+                        }))?;
+                        return Ok(());
+                    }


Inconsistent exit code when item is not found in non-JSON mode

When the item is missing and json_mode = true, a CliError::NotFound is returned, which the top-level handler maps to exit code 2. When json_mode = false, the code calls print_ok(false, …) and returns Ok(()), which exits with code 0.

For a destructive command like forget, scripts that do not use --json and rely on exit-code semantics will silently receive a success status even when the target item never existed. Emitting a user-readable message to stderr and using a non-zero exit in this branch (consistent with how Cmd::Sync handles misconfiguration) would make the behaviour match callers' expectations.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c97e7b455

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-17T01:18:30Z

crates/cli/src/main.rs

+                    if affected == 0 {
+                        if !json_mode {
+                            eprintln!("Error: memory item {} disappeared before deletion", id);
+                        }
+                        return Err(anyhow::anyhow!("item disappeared before deletion"));


Return NOT_FOUND on concurrent disappear in forget

When db.delete(&id) reports affected == 0 (the item was removed between the initial get and the delete), this branch returns a generic anyhow! error instead of CliError::NotFound. That causes wagl --json forget ... --confirm to emit RUNTIME_ERROR with exit code 1 instead of the stable NOT_FOUND/2 contract used for missing IDs, which can break automation that keys off not-found semantics in race conditions.

Useful? React with 👍 / 👎.

GoZumie force-pushed the feat/forget-command branch from 48bdd72 to 2165a0e Compare March 16, 2026 23:32

GoZumie marked this pull request as ready for review March 16, 2026 23:32

GoZumie requested a review from ChrisCompton as a code owner March 16, 2026 23:32

chatgpt-codex-connector bot reviewed Mar 16, 2026

View reviewed changes

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

GoZumie added 2 commits March 16, 2026 20:13

fix(db): include actionable flag in update_item SQL

79279ca

fix(cli): handle UTF-8 safely and improve forget command reliability

2c97e7b

chatgpt-codex-connector bot reviewed Mar 17, 2026

View reviewed changes

fix(cli): return NotFound error on concurrent deletion failure

47b7087

GoZumie changed the title ~~feat: add wagl forget <id> command to delete memories by ID~~ feat: add wagl forget command [DO NOT MERGE] Mar 17, 2026

GoZumie mentioned this pull request Mar 17, 2026

design: memory deletion policy — trust, safety, and integrity concerns #91

Open

ChrisCompton added the hold Do not take actions on this issue, or merge code related to this issue. label Mar 21, 2026

GoZumie mentioned this pull request Mar 24, 2026

feat: staged soft-delete pipeline with cooling periods #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add wagl forget command [DO NOT MERGE]#89

feat: add wagl forget command [DO NOT MERGE]#89
GoZumie wants to merge 4 commits intodevfrom
feat/forget-command

GoZumie commented Mar 16, 2026 •

edited by greptile-apps bot

Loading

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 16, 2026

Uh oh!

greptile-apps bot Mar 16, 2026

Uh oh!

greptile-apps bot Mar 16, 2026

Uh oh!

greptile-apps bot Mar 16, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	"text_preview": &item.text[..std::cmp::min(120, item.text.len())]
	"text_preview": item.text.chars().take(120).collect::<String>()

Conversation

GoZumie commented Mar 16, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GoZumie commented Mar 16, 2026 •

edited by greptile-apps bot

Loading