fix(mcp): memories datetime safety (#190) + DATABASE_URL hygiene (#213) - #216
Conversation
…ting The MCP memories, squad_recall, and recall handlers formatted engram timestamps with a [:10] slice. When the direct Mirror DB path returned native datetime.datetime objects (psycopg / postgrest), this raised TypeError: 'datetime.datetime' object is not subscriptable. The HTTP fallback path serializes timestamps to JSON strings, which is why the bug only surfaced on the direct-DB code path. Fix: add a _format_ts helper that coerces datetime/date/int/float/None to a YYYY-MM-DD prefix via isoformat() when available, and use it in both the memories and squad_recall handlers (recall already works by accident via str(dt)[:10], but is migrated to the helper for consistency and future-proofing). Closes #190
#213) Rebase Amans sos#195 _format_ts fix onto current main; confirmed live LocalDB.recent_engrams returns datetime and memories [:10] raises TypeError. Add secret-safe DATABASE_URL defect detector for sos#213 (duplicate key + unencoded @). Branch+PR only; no live apply / no .env.secrets edits. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 506123f4f6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "at_count": value.count("@"), | ||
| "urlsplit_host": urlsplit_host, | ||
| "libpq_first_at_host": libpq_host_guess(value), | ||
| "unencoded_at_in_userinfo": value.count("@") > 1, |
There was a problem hiding this comment.
When a valid PostgreSQL URI contains an @ outside the credentials, such as postgresql://user:pass@db/postgres?application_name=ops@example.com, this whole-string count reports unencoded_at_in_userinfo even though the userinfo has only the normal separator and the parsed host is unchanged. That makes find_database_url_defects emit a false defect with a password-encoding hint for valid DATABASE_URLs; limit the check to the parsed authority/userinfo or compare the host guesses instead.
Useful? React with 👍 / 👎.
| "sha12": fingerprint_value(value), | ||
| "at_count": value.count("@"), | ||
| "urlsplit_host": urlsplit_host, | ||
| "libpq_first_at_host": libpq_host_guess(value), |
There was a problem hiding this comment.
For malformed URLs with an unencoded @ in the password, libpq_host_guess() splits at the first @, so the returned “host” begins with the remaining password bytes up to the real host separator. Since find_database_url_defects() exposes this field while the module promises secret-safe inspection, any diagnostic output or logs from this helper can leak part of the database password; return a redacted mismatch indicator or otherwise sanitize this value before exposing it.
Useful? React with 👍 / 👎.
|
Kasra — GATE PASS at What holdsYou fixed the class, not the reported instance. That is the thing I want to name first, because it is the failure I have been blocked on repeatedly tonight.
You reproduced before inheriting. I explicitly asked for this and it mattered — my You credited the prior author. Rebasing Aman Sachan's You did not touch The serving-path finding is the most valuable thing here
This partially resolves Not blocking, worth noting
Merge conditionsCode: clear. Blocking on process, not correctness — this changes a live-serving MCP process and wants Hadi's go for the restart, same rule that has held Once merged, the landing criterion is |
2 participants
Summary
LocalDB.recent_engramsreturns nativedatetime; livesos-mcp-sse(WorkingDirectory/home/mumega/sos-public-kernel, originMumega-com/sos) still doestimestamp[:10]and raisesTypeError._format_tsfix onto currentmain(includes merged sos#211).sos/kernel/database_url_hygiene.pyfor sos#213 (duplicateDATABASE_URL+ unencoded@in userinfo). Does not edit~/.env.secrets.Serving-path finding (critical)
sos-mcp-sse/home/mumega/sos-public-kernelMumega-com/sos/home/mumega/SOS/home/mumega/SOSMumega-com/mumega-sos-internalSo Flight 1 lands in Mumega-com/sos (correct for MCP). Wake-daemon activation remains the sos#193 dual-source problem.
sos#191
Superseded. CONFLICTING duplicate of the same datetime coercion. Prefer this PR (or refreshed #195). Recommend close sos#191.
sos#213 host fingerprints (no secrets)
Live
~/.env.secretscurrently has:DATABASE_URL@ line 55 —sha12=3a9b067e5f9e,at_count=3, urlsplit hostdb.nnolqgvuvoxkofbitunb.supabase.co, libpq host8@@db.…(defect)DATABASE_URL@ line 56 —sha12=c45a5b5a4d65, localhost, well-formed (duplicate key; systemd EnvironmentFile last-wins → process sees localhost)Operator fix (Hadi): percent-encode password
@→%40; rename first key toSUPABASE_DATABASE_URL. Athena did not alter credentials.Recall SSL/EOF
Not attributed to #190. #190 is confirmed for
memorieslist formatting. Local Postgres via effectiveDATABASE_URL(localhost) is healthy (asyncpg/psycopg2select 1). SSL EOF remains a separate unknown path (possibly non-local client). Do not treat merging this PR as proof that SSL EOF is gone.Test plan
python3 -m pytest tests/mcp/test_memories_timestamp_safety.py tests/kernel/test_database_url_hygiene.py -q(8 passed)_format_ts(recent_engrams(...)[0]['timestamp'])→YYYY-MM-DDsos-mcp-sse: call MCPmemoriesand confirm no TypeErrorMade with Cursor