Skip to content

feat: add assistant preference memory#686

Merged
eloylp merged 2 commits into
mainfrom
fix/664-preference-memory
Jun 3, 2026
Merged

feat: add assistant preference memory#686
eloylp merged 2 commits into
mainfrom
fix/664-preference-memory

Conversation

@eloylp

@eloylp eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • add global assistant preference memory storage with active/proposed/rejected/archived lifecycle and evidence metadata
  • expose memory list/create/update/approve/reject/archive through REST, MCP, and the Improvements UI
  • feed approved memory into self-improvement analysis, show memory influences on recommendations, and propose memory from recommendation decisions without auto-activating it

Closes #664

Verification

  • go test ./internal/selfimprovement ./internal/store ./internal/workflow ./internal/daemon/observe ./internal/mcp
  • npm test -- --run src/app/improvements/page.test.tsx
  • go test ./...
  • npm test
  • npm run build
  • go test ./internal/store ./internal/selfimprovement -race
  • git diff --check

Note: npm ci reported the existing Next.js security advisory and npm audit findings; this PR does not change dependencies.

@eloylp eloylp added ai ready Maintainer signal: the agent fleet may work on this issue or PR ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

  • [high] internal/selfimprovement/service.go:97 — Issue Add preference memory for the self-improvement assistant #664 requires preference memory to learn from complete maintainer decisions, and the maintainer comment specifically narrows that to both single-asset proposals and reactive multi-asset proposal bundles. The PR only proposes memory from UpdateRecommendationStatus (accepted/rejected/deferred recommendation status changes); the bundle decision entry points here just forward to edit/reject/link/publish/discard helpers without creating proposed memory from edited bundle items, rejected items, linked-existing choices, published bundles, or discarded bundles. That leaves the new Support reactive multi-asset self-improvement proposal bundles #683 bundle workflow out of the memory source set, so preferences shown through proposal-bundle decisions are silently lost.
  • [medium] internal/selfimprovement/recommendations.go:171RecordRecommendation records every active memory row as memory_influences for every recommendation, regardless of whether the analyst cited memory_influence_ids or whether current feedback/clarification overrode the stored preference. That makes the UI/API claim a recommendation was memory-influenced even when the active memory had no actual effect, which undermines the issue requirement to show which memory entries influenced a recommendation and weakens the “current instructions override stored memory” guarantee. Please persist only the memory IDs the analyzer actually reports as influential, or leave the influence list empty when memory was not used.

Summary

The approval gate for inferred memory is present, but the implementation misses bundle-decision learning and over-reports active memory influence.

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Addressed the review findings in ac4e0c02:

  • Bundle decisions now propose preference-memory entries for edited items, rejected items, linked-existing choices, published bundles, and discarded bundles.
  • Recommendation recording now persists memory_influences only for active memory IDs explicitly reported by the analyst through memory_influence_ids; recommendations without reported IDs remain unmarked.

Verification:

  • go test ./internal/selfimprovement
  • go test ./...
  • git diff --check

@eloylp eloylp added the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The follow-up commit addresses the prior bundle-decision memory and explicit memory-influence persistence findings, and the linked issue goals are covered with tests and docs.

@eloylp eloylp added human review ready Passed AI review, ready for human merge decision and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Review recommendation

Please gate inferred memory on a real bundle-item edit.

Current path:

  • internal/selfimprovement/service.go:97 always calls proposeMemoryFromProposalBundleDecision(..., "edited_proposal_bundle_item") after UpdateProposalBundleItem.
  • internal/selfimprovement/bundles.go:375 builds the after item but does not compare it with the existing item.
  • internal/ui/src/app/improvements/page.tsx:601 exposes Save Item even when the draft is unchanged.

That means a no-op save can propose preference memory saying maintainers usually edit bundle items, even though the maintainer made no actual change. Since this PR is about learning preferences, no-op UI actions should not become evidence.

Suggested acceptance criteria:

  • Compare before and after material fields: proposed_ref, proposed_name, proposed_scope, proposed_body, proposed_description, proposed_enabled, proposed_position.
  • If unchanged, do not write an edit audit event and do not propose assistant memory.
  • Prefer disabling Save Item in the UI when unchanged, but the service should still be authoritative.
  • Add a regression test that a no-op bundle item save does not create an edited_proposal_bundle_item memory proposal.

@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Maintainer steering

For v1, be pragmatic and remove workspace separation from assistant preference memory.

Reasoning:

  • Improvements can affect prompts, skills, and guardrails in different catalog scopes.
  • The preference memory is about how the self-improvement assistant should reason, not about the workspace where feedback was captured.
  • Workspace-scoped memory would fragment assistant learning and make the same preference have to be relearned per workspace.

Requested direction:

  • Keep feedback and recommendations workspace-scoped, because those are evidence records tied to repo/workspace context.
  • Make assistant_memory global: remove workspace_id and the workspace FK/index from the new table.
  • Load active assistant memory globally for every agents.improvement analysis.
  • Remove the workspace filter/selector from memory REST, MCP, and UI surfaces, or ignore workspace there if wire compatibility is easier.
  • Deduplicate memory keys globally.
  • Preserve origin/context through evidence_type, evidence_id, evidence_url, and the linked recommendation/feedback data.

If scoped memory becomes necessary later, add explicit scope_type/scope_ref. The v1 default should be global assistant preference memory.

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed human review ready Passed AI review, ready for human merge decision labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

  • [high] internal/store/migrations/043_assistant_preference_memory.sql:3 — The maintainer’s latest steering requests global assistant preference memory for v1, but this schema still makes every assistant_memory row workspace-scoped with a required workspace_id, workspace FK, and workspace indexes. The store also filters list calls by workspace_id, so active memory remains fragmented per workspace instead of being loaded globally for every improvement analysis.
  • [medium] internal/selfimprovement/service.go:102 — The maintainer asked to gate inferred memory on a real bundle-item edit, but UpdateProposalBundleItem still proposes edited_proposal_bundle_item memory after every successful save without comparing the before/after material fields. A no-op save can still become preference-memory evidence, and there is no regression test proving no-op bundle item saves avoid memory proposals.
  • [medium] internal/daemon/observe/observe.go:358 — The unresolved maintainer thread asks not to add combined method-switch handlers and to split handlers per HTTP method. This PR still routes GET and POST /improvements/memory to one HandleImprovementMemory switch, so that requested handler shape has not been addressed.

Summary

The previous review findings were fixed, but the new maintainer steering changes the acceptance target: memory needs to be global for v1, no-op bundle edits must not create evidence, and the unresolved handler-structure request needs resolution.

@eloylp eloylp removed the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Addressed the latest maintainer/reviewer feedback in b20435f:

  • Made assistant_memory global for v1 by removing workspace_id, workspace FK/indexing, and workspace filters from REST/MCP/UI memory surfaces; live memory keys now dedupe globally.
  • Gated bundle-item edit memory on real material changes and added a regression test that no-op saves do not write edit audit events or propose edited_proposal_bundle_item memory.
  • Split GET and POST /improvements/memory into separate handlers.

Verification:

  • go test ./internal/selfimprovement ./internal/store ./internal/daemon/observe ./internal/mcp
  • npm test -- --run src/app/improvements/page.test.tsx
  • go test ./...
  • npm test
  • npm run build
  • git diff --check

Note: npm ci reported the existing Next.js security advisory and npm audit findings while installing test dependencies.

@eloylp eloylp added the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

  • [medium] docs/self-improvement-feedback.md:96 — The latest maintainer steering says v1 assistant preference memory should be global, but the docs still describe it as “workspace-scoped product memory.” The schema/store changes now appear to make memory global, so this documentation would teach operators the opposite model and misses the requested direction to avoid workspace-fragmented learning.

Summary

The latest code changes address the behavior-level feedback I checked, but the docs still need to reflect global assistant preference memory for v1.

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Addressed the latest docs review finding in 95cc66b:

  • Updated docs/self-improvement-feedback.md to describe assistant preference memory as global v1 product memory, while keeping feedback and recommendations workspace-scoped as evidence records.

Verification:

  • go test ./...
  • git diff --check

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The follow-up docs change now describes assistant preference memory as global v1 product memory while preserving workspace-scoped feedback and recommendation evidence, and the implementation/docs are consistent with the maintainer steering.

@eloylp eloylp added human review ready Passed AI review, ready for human merge decision and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The latest commit updates the docs to describe assistant preference memory as global v1 product memory while preserving workspace-scoped feedback/recommendation evidence, and the final code/docs wording is consistent with the maintainer's global-memory steering.

@eloylp eloylp added the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Follow-up after CI caught the newer main strict-schema test:

  • Rebased the PR onto current main.
  • Updated the self-improvement recommendation schema/prompt so memory_influence_ids and memory_influence_rationale are always present, using empty values when memory did not influence the recommendation.

Verification after rebase:

  • go test ./internal/workflow -run TestSelfImprovementRecommendationSchemaIsStrict -count=1
  • go vet ./...
  • go test ./... -count=1
  • cd internal/ui && npm ci && npm run build
  • git diff --check

Note: npm ci still reports the existing Next.js security advisory and npm audit findings.

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed human review ready Passed AI review, ready for human merge decision ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The rebase/follow-up keeps the assistant memory schema strict with empty memory influence fields when unused, adds the requested go-api guidance across the fleet examples/docs, and remains consistent with the linked issue goals plus later maintainer steering.

@eloylp eloylp added human review ready Passed AI review, ready for human merge decision ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The latest head satisfies the linked issue goals and later maintainer steering: assistant preference memory is global and human-gated, recommendation influence reporting is explicit, bundle-decision learning avoids no-op edit evidence, REST/MCP/UI/docs are covered, and the requested go-api guidance is in the fleet examples/docs.

Verification: go test ./internal/selfimprovement ./internal/store ./internal/workflow ./internal/daemon/observe ./internal/mcp, go test ./internal/selfimprovement -race, and git diff --check origin/main...HEAD passed locally.

@eloylp eloylp added ai inprogress Visible semaphore: an agent is actively working on this issue or PR and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

  • [medium] internal/daemon/observe/observe.go:108 — The maintainer’s latest inline steering asked that Go API routes split “one per each http method,” and this PR’s new go-api skill now says each HTTP method should route to a distinct handler function. The new memory archive route still registers both POST and DELETE on the same HandleArchiveImprovementMemory handler, and the docs expose that as POST|DELETE /improvements/memory/{id}/archive. That leaves the newly added endpoint inconsistent with the requested API shape. Please either keep only one archive/delete method or split the two methods into distinct handlers/routes and cover the chosen shape in tests/docs.

Summary

The focused Go test suite passed, and the memory behavior appears aligned with the issue goals, but the new API guidance is not fully applied to the archive endpoint.

@eloylp eloylp removed human review ready Passed AI review, ready for human merge decision ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp eloylp force-pushed the fix/664-preference-memory branch from bd06435 to 9d3d79b Compare June 3, 2026 21:36
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Rebased PR #686 onto current main and force-pushed 9d3d79b.

Conflict resolution kept the existing published-skill linking flow from main alongside the assistant preference memory UI and explicit memory-influence handling. I also removed the stale deferred recommendation memory path because current main only accepts accepted/rejected human recommendation decisions.

Verification after rebase:

  • go test ./...
  • npm test -- --run src/app/improvements/page.test.tsx
  • git diff --check

CI is green on the new head.

@eloylp eloylp added the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
Agents-Run: c3d3fd7b146edb06

Agents-Agent: coder
@eloylp eloylp force-pushed the fix/664-preference-memory branch from 9d3d79b to fb90cb2 Compare June 3, 2026 21:41
Agents-Run: eb021c54ec668043

Agents-Agent: coder
@eloylp eloylp force-pushed the fix/664-preference-memory branch from 9b955a3 to 057b2d4 Compare June 3, 2026 21:46
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Addressed the archive route review finding in the current head 9b955a3:

  • Kept POST /improvements/memory/{id}/archive as the single REST archive route and removed the combined DELETE registration.
  • Updated docs/self-improvement-feedback.md to document the POST-only archive endpoint.
  • Added router coverage for the memory routes plus a negative check that DELETE /improvements/memory/{id}/archive is not registered.

Verification:

  • go test ./internal/daemon ./internal/daemon/observe
  • go test ./...
  • npm test -- --run src/app/improvements/page.test.tsx
  • git diff --check

CI is green on 9b955a3.

@eloylp eloylp removed the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Maintained PR #686 after the latest rebase/review activity and pushed 057b2d4.

  • Kept the archive endpoint on a single POST /improvements/memory/{id}/archive route per the Go API handler guidance already present on the current head.
  • Kept the existing published-skill linking flow type-safe by using the attributed feedback workspace for the agent update URL.
  • Removed the accidental deferred-status acceptance from my intermediate maintenance patch; current recommendation decisions remain accepted/rejected only.

Verification:

  • go test ./internal/selfimprovement ./internal/workflow ./internal/daemon/observe ./internal/mcp
  • npm test -- --run src/app/improvements/page.test.tsx
  • go test ./...
  • npm run build
  • git diff --check

CI is green on 057b2d4.

@eloylp eloylp added the ai inprogress Visible semaphore: an agent is actively working on this issue or PR label Jun 3, 2026
@eloylp

eloylp commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review — pr-reviewer

Verdict: APPROVE

Findings

  • None.

Summary

The current head keeps the archive endpoint POST-only with negative DELETE route coverage, preserves global human-gated assistant preference memory, and remains aligned with issue #664 plus the later maintainer steering.

Verification: go test ./internal/selfimprovement ./internal/store ./internal/workflow ./internal/daemon/observe ./internal/mcp ./internal/daemon and git diff --check origin/main...HEAD passed locally.

@eloylp eloylp added human review ready Passed AI review, ready for human merge decision and removed ai inprogress Visible semaphore: an agent is actively working on this issue or PR labels Jun 3, 2026
@eloylp eloylp merged commit c7cbf28 into main Jun 3, 2026
1 check passed
@eloylp eloylp deleted the fix/664-preference-memory branch June 3, 2026 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai ready Maintainer signal: the agent fleet may work on this issue or PR human review ready Passed AI review, ready for human merge decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add preference memory for the self-improvement assistant

1 participant