awslabs / awslabs/cli-agent-orchestrator
[Bug] store_lesson still reports filesystem faults as "learning disabled" and opens SQLite in the agent process
- 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
Assessment
This issue has not been assessed yet.