awslabs / awslabs/cli-agent-orchestrator

[Bug] store_lesson still reports filesystem faults as "learning disabled" and opens SQLite in the agent process

Open
#744 1 comment 0 reactions 1 assignee Claimed by @gutosantos82 View on GitHub
bug
Dominant language
Python
Stars
1.3k
Forks
267
Avg merge
1d 23h
Merged PRs (30d)
70

Description

`store_lesson` is the one learning-gated MCP tool that #734 leaves behind, and it retains exactly the two behaviours that PR removes from `report_outcome` and `list_outcomes`:

1. **It gates in-process.** It calls `is_learning_enabled()` directly, and that function fails closed on a read error — so an unreadable `settings.json` still resolves to `{"success": false, "disabled": true}`. Both copies of `skills/cao-learning` instruct agents to skip a `disabled: true` payload **silently**, so a filesystem fault still presents itself as a deliberate opt-out and the lesson vanishes with no error anywhere.

2. **It opens SQLite in the agent's process** via `MemoryService()` rather than going over HTTP, so it fails for any agent that does not share a filesystem with cao-server — the same coupling #734 removes from the outcome tools.

This was deliberately scoped out of #734 and that was the right call for review size. Filing it so the deferral is tracked rather than implicit: after #734 the honesty rationale applies inconsistently across the three learning-gated tools, which is worse than applying nowhere — an operator who learns to trust `report_outcome`'s error reporting will reasonably assume `store_lesson` behaves the same way.

## Why it is not a trivial copy of the #734 change

`report_outcome` and `list_outcomes` had `POST`/`GET /outcomes` waiting for them. `store_lesson` has no HTTP equivalent today: it writes a lesson into a **named worker profile's** agent scope, which is a different shape from the memory routes that exist (`/memory`, `/memory/{key}`, …) because the target scope is an argument rather than the caller's own context. So this needs a route designed, not just a client rewired.

That also makes it entangled with the first open question in #733 — whether the server should resolve scope from its own terminal record instead of accepting a client-supplied `terminal_context`. If that lands, `store_lesson`'s route should be designed on top of it rather than duplicating the client-side resolution.

## Also worth noting

#734's shared context-lookup change quietly altered `store_lesson`'s down-server error text (it now surfaces a raw exception string), and that path is untested. Worth a test whichever way this goes.

## Acceptance

- [ ] An HTTP route that can write a lesson to a named agent profile's scope
- [ ] `store_lesson` calls it instead of instantiating `MemoryService()`
- [ ] An unreadable `settings.json` surfaces as a plain error, never `disabled: true`
- [ ] A missing route / unreachable server is an explicit failure, not `disabled` (mirror the `LEARNING_DISABLED_CODE` discriminator from #734 — do not infer feature state from a bare 404)
- [ ] Tests for the gate-404 vs route-404 split, and for the down-server error text

Depends on the scope-resolution decision in #733.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.