feat(memory): FEAT-019 memory capability, memory facet, and typed outcomes (#5609) - #5833
feat(memory): FEAT-019 memory capability, memory facet, and typed outcomes (#5609)#5833Hmbown wants to merge 14 commits into
Conversation
…es to command contract
- Restore CommandCapabilities bitset with MEMORY bit (D1/D3) and Contextual { capabilities, handler } shape
- Add CommandMemoryContext facet with typed per-operation methods (D1/D9)
- Add contract-owned values: MemoryHit, MemoryStatus, MemoryRemembered, MemoryImportOutcome, MemoryGetOutcome, MemoryExport, MemoryReindex, MemoryDelete, MemoryRememberTarget, MemoryDeleteScope (D2)
- Add memory slot to CommandContexts/ContextParts with duplicate-slot rejection
- Contract tests: object safety, typed results, workspace scoping (D8), exact capability declarations, envelope transport
Generated with Claude Code
(cherry picked from commit e4b9621)
… fake Generated with Claude Code (cherry picked from commit e2a93bf)
…nd utility capability declarations
- Add MemoryAdapter implementing CommandMemoryContext over App memory fields + NativeMemoryStore (D9)
- Add capability-driven bundle contexts(capabilities) with MEMORY slot and restricted exposure (D1/D3)
- Update dispatcher to destructure Contextual { capabilities, handler } and populate only declared slots
- Declare exact capabilities for existing utility handlers (attach=WORKSPACE|MEDIA, automation/mcp=PRESENTATION, task=WORKSPACE) with safe missing-facet errors
- Adapter tests: path/enablement, status/path, workspace identity, search/remember/get/export/reindex, import imported/skipped, scoped deletes, restricted exposure, no eager I/O
Generated with Claude Code
(cherry picked from commit 1f0ce5b)
…extual dispatch Generated with Claude Code (cherry picked from commit 48109c9)
…lers
- /note: Contextual { capabilities: WORKSPACE }, handler-owned .codewhale/.deepseek fallback (D3); 12 tests ported to FakeWorkspace + .codewhale preference test
- /memory: Contextual { capabilities: WORKSPACE | MEMORY }, full native matrix over the facet; 3 tests ported + every D6/D9 branch (status, path, search, remember global/workspace, import, get, export, reindex, delete all/global/workspace, missing facets)
- Register both via ContextualCommand::from_contract in the group registry
- Boundary audit: CommandResult is the only TUI-owned data reference; no App/store/hit/anyhow/action in production handlers (D4)
Generated with Claude Code
(cherry picked from commit fe9b8e9)
… public dispatch - Remove memory from PENDING_GROUPS projection and command-migration-topology.json frontier together (D7 all-or-nothing) - Update migration fixture to the seven-group frontier - Public registry/dispatch tests: exact capability declarations for /note (WORKSPACE) and /memory (WORKSPACE | MEMORY), no presentation/media, metadata bridging, real dispatch through the seam, no panics - Live gate: pure shrink accepted; source scan confirms memory group has no concrete-App handlers Generated with Claude Code (cherry picked from commit 11944f1)
The envelope grew to ten facets (session, model, cost, mode_policy, system_prompt, skills, workspace, presentation, media, memory). The stale 'seven facets' comments were the exact hygiene item flagged in the FEAT-018 PR review (Lstarsky0, #5525) and recorded as FEAT-019 D11. (cherry picked from commit 93aa513)
FEAT-019 changed CommandHandler::Contextual(fn) to Contextual {
capabilities, handler }. /loop landed on this branch just before that
conversion and was missed when utility handlers declared exact
capabilities, so the watcher command no longer compiled.
Match /automation: PRESENTATION only, a safe missing-facet error, and a
public-seam test that still creates minute-level watchers.
Co-Authored-By: Grok 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_79ef0a4d-7edd-45df-ab14-d8ac7513ae1b) |
|
Claude finished @Hmbown's task in 3m 28s —— View job Claude finished reviewing this PR
SummaryThis branch does not build. That's the headline finding — everything else is secondary until it's fixed. 1. Unresolved git merge-conflict markers committed to the branch (blocks compilation)Three files carry literal
2.
|
… project handlers
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_29e60fb8-2953-4c0c-8194-046ba95a4f6d) |
Review in progress
|
There was a problem hiding this comment.
🟡 Changes recommended
There are unresolved merge-conflict markers (non-compilable state) and a newly added /loop test/topology entry that references a command/file that does not exist.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR re-lands FEAT-019 “memory commands” work by adding a MEMORY capability bit to the command contract, introducing a typed CommandMemoryContext facet, and migrating /note and /memory onto capability-scoped contextual handlers with a TUI host adapter.
Changes:
- Adds
CommandCapabilities(includingMEMORY) and wires capability-driven envelope construction so contextual handlers receive only declared facets. - Introduces the contract-level memory facet + typed outcomes, plus a TUI
MemoryAdapterthat maps native-memory operations into the portable contract types. - Migrates the memory command group (
/note,/memory) and updates migration topology/frontier expectations accordingly.
File summaries
| File | Description |
|---|---|
| scripts/test_check_command_migration_manifest.py | Updates the expected pending-frontier set after removing memory from the migration frontier. |
| scripts/command-migration-topology.json | Updates topology scope lists and removes memory from the pending frontier. |
| crates/tui/src/commands/mod.rs | Switches contextual handler registration/dispatch to capability-scoped envelopes; adds/updates dispatch tests. |
| crates/tui/src/commands/groups/utility/task.rs | Declares required capabilities and replaces .expect() facet access with safe missing-capability errors. |
| crates/tui/src/commands/groups/utility/mcp.rs | Declares presentation capability and uses safe missing-capability errors. |
| crates/tui/src/commands/groups/utility/automation.rs | Declares presentation capability and uses safe missing-capability errors. |
| crates/tui/src/commands/groups/utility/attachment.rs | Declares workspace+media capabilities and uses safe missing-capability errors. |
| crates/tui/src/commands/groups/memory/note.rs | Converts /note to a contextual handler requiring WORKSPACE and removes direct App dependency. |
| crates/tui/src/commands/groups/memory/mod.rs | Registers memory group commands via ContextualCommand::from_contract. |
| crates/tui/src/commands/groups/memory/memory.rs | Converts /memory to contextual (WORKSPACE |
| crates/tui/src/commands/contract.rs | Adds TUI-side memory adapter and capability-driven envelope wiring (but currently contains unresolved merge conflicts). |
| crates/command-contract/src/tests.rs | Adds contract-level tests for memory facet/capabilities (but currently contains unresolved merge conflicts). |
| crates/command-contract/src/lib.rs | Re-exports CommandCapabilities from the contract crate. |
| crates/command-contract/src/handler.rs | Adds CommandCapabilities, extends CommandHandler::Contextual to carry capabilities, and adds the memory facet slot to CommandContexts. |
| crates/command-contract/src/facets.rs | Adds the memory facet trait and typed outcomes (but currently includes a leftover merge-conflict marker). |
Review details
Suppressed comments (6)
crates/command-contract/src/tests.rs:432
- A second unresolved merge-conflict block begins here (project facet impl vs memory facet impl). This must be resolved (and conflict markers removed) for tests to compile; after merging, both CommandProjectContext and CommandMemoryContext fakes/impls should exist as separate types.
impl FakeMemory {
fn new() -> Self {
Self {
hits: vec![MemoryHit {
source: PathBuf::from("/mem/source.md"),
line_start: 3,
line_end: 5,
crates/command-contract/src/tests.rs:635
- Another unresolved merge-conflict marker appears before the project facet tests. After resolving earlier conflicts, ensure the project tests remain present and are not accidentally nested inside the memory test additions (remove conflict markers and restore proper test/module structure).
fn project(_: &dyn CommandProjectContext) {}
project(&FakeProject::new());
let mut project = FakeProject::new();
assert!(!project.lsp_enabled());
crates/tui/src/commands/contract.rs:948
- Unresolved merge-conflict markers in CommandContextBundle field list (project vs memory). After merging, this bundle should include both
project: ProjectAdapterandmemory: MemoryAdapterfields (plus the other facets) without conflict markers.
/// Expose exactly the capabilities declared by the command registration.
pub(crate) fn contexts(&mut self, capabilities: CommandCapabilities) -> CommandContexts<'_> {
let mut contexts = CommandContexts::empty();
if capabilities.contains(CommandCapabilities::SESSION) {
contexts = contexts.with_session(&mut self.session);
crates/tui/src/commands/contract.rs:1041
- Unresolved merge-conflict markers in App::command_contexts() bundle construction (project vs memory/media initialization). This must be merged so all adapter fields in CommandContextBundle are initialized exactly once and compilation succeeds.
/// A 1x1 PNG for media adapter tests.
const PNG_1X1: &[u8] = &[
0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44,
0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f,
0x15, 0xc4, 0x89, 0x00, 0x00, 0x00, 0x0a, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x63, 0x00,
crates/tui/src/commands/contract.rs:1522
- Unresolved merge-conflict markers in the tests (project adapter tests vs memory adapter tests). Remove the conflict markers and ensure both test suites remain (or consciously prune one) so the module compiles and coverage is preserved.
let mut parts = bundle.parts();
let presentation = parts.presentation.as_mut().expect("presentation facet");
let accepted = presentation
.translate("goal_control_accepted", &[])
.expect("goal_control_accepted must resolve");
crates/tui/src/commands/contract.rs:971
- This merge-conflict block leaves two incompatible
contextsAPIs (contexts()vscontexts(capabilities)). The dispatcher now callscontexts(capabilities), so the legacycontexts()branch should be removed and the capability-scoped method should also conditionally include PROJECT (in addition to MEMORY).
}
if capabilities.contains(CommandCapabilities::PRESENTATION) {
contexts = contexts.with_presentation(&mut self.presentation);
}
if capabilities.contains(CommandCapabilities::MEDIA) {
- Files reviewed: 20/20 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn attach_media(&mut self, resolved_path: &Path) -> Result<MediaAttachmentReceipt, String>; | ||
| } | ||
|
|
||
| <<<<<<< HEAD |
| use codewhale_command_contract::facets::{ | ||
| <<<<<<< HEAD | ||
| CommandCostContext, CommandMediaContext, CommandModePolicyContext, CommandModelContext, | ||
| CommandPresentationContext, CommandProjectContext, CommandSessionContext, CommandSkillsContext, | ||
| CommandSystemPromptContext, CommandWorkspaceContext, MediaAttachmentReceipt, ProjectGoalState, |
| // /loop (contextual, presentation facet): interval + prompt creates | ||
| // a minute-level watcher through the same AutomationAction path. | ||
| let loop_create = execute("/loop 45m continue the market-readiness handoff", &mut app); | ||
| match loop_create.action { | ||
| Some(crate::tui::app::AppAction::Automation( | ||
| crate::tui::app::AutomationAction::Create { | ||
| prompt, | ||
| rrule, | ||
| interval_label, | ||
| .. | ||
| }, | ||
| )) => { | ||
| assert_eq!(prompt, "continue the market-readiness handoff"); | ||
| assert_eq!(rrule, "FREQ=MINUTELY;INTERVAL=45"); | ||
| assert_eq!(interval_label, "45m"); | ||
| } | ||
| _ => panic!("expected /loop create, got {loop_create:?}"), | ||
| } |
| "crates/tui/src/commands/groups/utility/automation.rs", | ||
| "crates/tui/src/commands/groups/utility/jobs.rs", | ||
| "crates/tui/src/commands/groups/utility/loop_cmd.rs", | ||
| "crates/tui/src/commands/groups/utility/mcp.rs", | ||
| "crates/tui/src/commands/groups/utility/network.rs", |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f4778586-442d-482e-86a3-a68f219fbd8f) |
|
Claude finished @Hmbown's task in 2m 30s —— View job Claude finished re-reviewing this PR
SummaryThe three follow-up commits fix the compile-breaking issues from the last review. One issue from that review is only half-fixed and will still fail CI. Fixed, confirmed by direct source inspection:
Still open — will fail CI (not addressed by the fix commits):
This is the same root cause as before (the Design notes (unchanged from prior review)The capability-scoped dispatch design remains solid: Not evaluatedNo |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a1ec539c-4d5e-4bb5-87d9-96661ee17ecc) |
|
Claude finished @Hmbown's task in 2m 20s —— View job Claude finished re-reviewing this PR
SummaryGood progress since the last review pass — the compile-breaking merge conflicts and stale tuple-variant call sites are all genuinely fixed. One issue flagged in both prior review rounds is still unresolved and will fail CI. Confirmed fixed:
Still open — will fail CI (flagged in both prior review rounds, not addressed by any of the three follow-up commits):
Minor, non-blocking:
Design notes (unchanged from prior reviews)The capability-scoped dispatch design remains solid: least-authority facet exposure, correct memory/workspace boundary, and safe fallbacks instead of panics in the newly-converted handlers. No provider/model-prefix inference issues — this PR doesn't touch model/provider selection. Not evaluatedNo |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR re-lands FEAT-019 memory capability by adding a CommandMemoryContext facet, a CommandCapabilities bitmask on contextual handlers, a TUI MemoryAdapter, and migrating /note and /memory to portable contextual handlers. Test coverage is substantial across contract fakes, adapter integration, and public registry dispatch.
Findings
- [WARNING] Some migrated contextual handlers may still panic on missing facets
The dispatcher now populates only declared capabilities. This diff updates attach, automation, mcp, task, memory, and note to return safe errors when a declared facet is absent. However, the bodies of goal, init, lsp, share, dispatch, and feat015 contextual handlers are not shown in this diff and may still useexpecton their facets. If a capability declaration is ever incomplete, dispatch will panic. Recommend converting these handlers to the same safelet Some(...) elsepattern and adding negative tests with emptyCommandContexts. - [INFO] Duplicated doc comment for CommandProjectContext (
crates/command-contract/src/facets.rs)
A new doc comment block was inserted immediately before an existing nearly identical doc comment for the project command group. The resulting duplicate documentation is confusing and should be collapsed to a single comment. - [INFO] CommandCapabilities::contains(NONE) is always true (
crates/command-contract/src/handler.rs)
The expressionself.0 & capability.0 == capability.0evaluates to true whencapabilityisCommandCapabilities::NONEbecause(self & 0) == 0. While no current callers passNONE, this is likely an unintended semantic for acontainsmethod and may hide bugs if such a check is introduced later.
Assessment
The PR is well structured and heavily tested. The capability-gated dispatch and typed memory outcomes are solid improvements. The main residual risk is that not all migrated contextual handlers were shown to be converted to safe facet handling; this should be verified before merge to avoid runtime panics through the public dispatch path.
Advisory review by Codewhale (codewhale review --pr 5833 --post, head 0ab0f07cd0a17ddb9fdc5af9a04dce2a336c4d9c). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
Remove the orphaned loop topology entry that fails the migration gate, enforce exact project authority, make contextual fixtures fail closed, and pin empty capability containment semantics. Signed-off-by: Paulo Aboim Pinto <paulo.aboim.pinto@gmail.com>
Preserve the FEAT-019 memory facet and capability-scoped dispatch while integrating current main, including FEAT-022 skill-group handlers and the control-socket changes. Resolve the combined migration frontier to five pending groups. Signed-off-by: Paulo Aboim Pinto <paulo.aboim.pinto@gmail.com>
|
Update: #5839 is mergeable with a clean merge state and its latest required checks are green. The outstanding review findings are resolved in #5839, which targets this PR's Resolved:
Validation on the reconciled branch:
Our account has read-only access to the maintainer-owned branch, so merging #5839 is the required branch update. That will apply the review fixes, update #5833 to current Paulo Aboim Pinto |
|
Babysitting update: #5833 is still on old head I attempted to convert #5833 to draft, but GitHub rejected the operation because Owner action required:
Please do not merge the current Paulo Aboim Pinto |
Pull request was converted to draft
fix(commands): resolve FEAT-019 review findings
|
Matrix is green on the updated head (merge state CLEAN). Leaving the Ready flip and merge to you per your review. |
|
Confirmed independently on updated head
I attempted the Ready flip after this verification, but GitHub rejected it:
The repository grants this account read-only permission, so only the owner/maintainer can move #5833 out of draft and merge it. Please mark it Ready for Review now. The PR is green, clean, and ready to merge; after the Ready transition, please merge it using the repository's configured strategy. Paulo Aboim Pinto |
Closes #5609
Re-lands the FEAT-019 memory commands slice from
codex/v0912-integration-20260823.CommandCapabilities::MEMORYcapability bit andCommandMemoryContextfacet/noteand/memoryto portable contextual handlersHarvested from PR #5609 by @aboimpinto
Co-authored-by: Paulo Aboim Pinto paulo.aboim.pinto@gmail.com
Note
Medium Risk
Changes the shared contextual dispatch envelope and migrates memory commands that mutate persistent native-memory storage, though behavior is heavily regression-tested for parity with the legacy handlers.
Overview
Introduces FEAT-019 on the command contract: a
CommandMemoryContextfacet with portable typed outcomes (search, remember, get, import/export, reindex, scoped delete), plus aCommandCapabilitiesbitmask so each contextual handler declares least authority.CommandHandler::Contextualnow carries{ capabilities, handler }instead of a bare function pointer. Dispatch andCommandContextBundle::contextsonly wire the facet slots a command registered—memory is no longer always injected with every adapter.On the TUI side,
MemoryAdaptermapsApp/NativeMemoryStoreto those contract types (legacy store path derivation preserved)./noteand/memorymove offAppto portableRegisterCommandhandlers (WORKSPACEvsWORKSPACE | MEMORY). Migrated contextual commands return safe errors when a declared facet is missing instead of panicking viaexpect. Migration topology andPENDING_GROUPSdrop memory from the pending frontier; tests cover contract fakes, adapter integration, and public registry dispatch.Reviewed by Cursor Bugbot for commit 0ab0f07. Bugbot is set up for automated code reviews on this repo. Configure here.