Skip to content

feat(doctor): amem doctor report and sync cadence without a daemon (Refs #40 F15, F16) - #52

Merged
entelecheia merged 1 commit into
mainfrom
feat/40-s4a-doctor
Oct 24, 2026
Merged

entelecheia merged 1 commit into
mainfrom
feat/40-s4a-doctor

Conversation

@entelecheia

@entelecheia entelecheia commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

What

agent-mem#40 F15, F16 and AC8.

  • F15 amem doctor [--json]: one read-only report, nine sections in a fixed order, each ok, attention or unavailable (with a reason, never a silent zero): duplicates, stale notes, pending candidates, capture freshness and hook registration per CLI, the brief's token cost, index equals rebuild, reader leases, the policy in effect, and the ingest and sync cadence. New src/memory/doctor.ts, a doctor branch in src/cli.ts right after status, and a command-table entry.
  • AC8: --json is amem.doctor/v1. test/doctor-snapshot.test.ts pins every key, value type and the fixed vocabulary against test/fixtures/doctor-snapshot.json, for a populated and an unused home.
  • F16 ingest: nothing new to build. #417's setup already registers amem ingest <cli> at SessionEnd (Codex, Kimi) and session_shutdown (Pi), and Claude has its hooks. Doctor reads the setup receipts and reports a CLI with no registered hook as manual, with the command to register it.
  • F16 sync (claude's ruling B+C, owner decision O3): amem registers no sync hook and no resident process. amem sync --if-due [--force] is the person's opt-in entry from their own shell. It does nothing without a configured remote, nothing when not due (hourly after a success, 15 minutes doubling to a day after failures), and stops after 30 seconds. --force needs --if-due. State is sync-state.json (two times and a failure count, never an error text, mode 0600), stamped by every amem sync. Doctor shows the remote as configured or none (never the URL), last attempt and success, failures and next attempt. Option A (a detached sync child from the hook) is rejected in docs/contracts.md with the reasons.

Read-only, measured

Doctor writes nothing into the memory home. Every check that needs a store runs on a private scratch copy in a temporary directory that is removed afterwards: the project's (and its aliases') curated logs, the lifecycle and alias logs, and the project's work logs in a base of their own. The live shared-v1 index is only snapshotted (a read-only connection and VACUUM INTO), brought level with the copied logs as the next reader would, and compared with a full rebuild of the same logs. A curated split or generation home reports skipped for that (its build already proves equality). The brief is measured through sessionStartBrief({ measureOnly }), used by doctor alone, which takes no reader lease and reads only caller-supplied scratch stores. Without a scratch work base the work section is unavailable, never complete.

test/doctor.test.ts hashes every file of a populated home before and after the FIRST run (no warm-up), -wal included. The one thing not compared is a -shm file's content (SQLite's wal-index scratch memory, changed by any read-only open); its presence and size are compared. A home nobody used stays absent. Further tests: a foreign machine segment with the live index behind the logs (untouched), a deleted work mutation-lock.db (stays deleted), and a work log with no index (the item is still in the brief).

Review round 1 (claude, c8a5b23)

Fixed: (1) doctor refreshed the live index and took a lease: now every check runs on the scratch copy, tests hash the first run. (2) the brief's work list ran its migration: now a scratch work base. (3) the brief under-reported when work/index.db was absent: the scratch base replays the log. (4) sync --if-due --json stamped a failed sync as a success: the real result is read. (5) the timeout left git's ssh child running: git runs in its own process group when a stop signal is given, and Ctrl-C and SIGTERM now feed that signal (tested with the real binary under a pty: SIGINT to amem kills the transport child; a direct test of the abort too). (6) a generation home's remote was invisible: remoteConfigured resolves the active profile (this was also the bot's P2, answered and resolved).

Nits taken: a future stamp is no history; only a transport failure is stamped (lock contention, no remote, a policy decline, a public remote and usage errors are not); hookEvidence: "setup receipt" labels what "registered" means; a split or generation home's index section is skipped and no longer degrades the overall status; the report says which brief it measures and when the 100 000-note read is truncated.

Not fixed, stated: the stamp's read-modify-write is unlocked (two syncs finishing together can lose one count, which delays a backoff by one step); the 30 s bounds git steps but not the gh visibility lookup (its own 15 s) or the cleanup merge --abort (up to 60 s), which deliberately ignore the stop so the repository is left clean. Both are in docs/contracts.md.

Also changed: src/memory/sync.ts (the git spawn, only when a stop signal is passed) and ptyArgv moved from test/review-pending.test.ts to test/fakes/pty.ts so two test files share it.

Review round 3

sync --if-due runs git in its own process group, so the terminal's signals no longer reach git. Fixed as claude specified: the stop now covers SIGINT, SIGTERM, SIGHUP (terminal close, also how an EXIT trap's shell goes away) and SIGQUIT; handlers use on, not once, so a second Ctrl-C is not the default action that ends amem before git; and a process that exits for any other reason signals every live git group first (an exit handler installed when the first deadline git starts). Tests with the real binary under a pty: each of the four signals, three Ctrl-Cs in a row, a group SIGHUP as a closing terminal sends it, plus a process that dies by an uncaught throw with git running. Mutants: the old two-signal list fails 4 tests, once fails the unit test, and removing the exit handler fails the throw test. Rebuilt on main 206aa37 (#51), which touched none of my files. Ubuntu CI on an earlier head failed three doctor read-only tests because Linux recreates empty -wal and -shm files on a read-only open; the test now removes both first and normalises only those, which is explained in docs/contracts.md. Stated: SIGKILL of amem itself cannot be handled by anyone, so a git already running then has nothing to stop it; --if-due does not widen that old hazard of any killed sync.

One existing test changed on purpose

test/completion.test.ts lists the flags a raw hub-shared command may not accept. sync --if-due --force are handled in src/cli.ts before the raw command (which never sees them: checked), so they join the named exemptions, like search --all-projects.

Check

tsc clean, boundary OK (73 files), npm pack --dry-run 81 files, bun test --timeout 20000 1300 pass / 0 fail / 2 skip (on main 206aa37), hooks budget pass. 26 deliberate breaks each fail a test: 14 in the first round, and nine since (git in no process group, --json failure read as success, local refusals stamped, a future stamp trusted, no work base claimed complete, the live curated store opened, the live work list read, the live index not snapshotted, Ctrl-C not turned into the stop signal). One of those survived at first: removing the unavailable guard changed nothing visible because the work log was absent, so I added a brief-level test and the guard now fails it when it claims complete.

Refs #40

Copilot AI balanced review requested due to automatic review settings October 24, 2026 13:41
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T13:47:21.607570Z c8a5b23 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8a5b23d38

ℹ️ 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".

Comment thread src/memory/sync-cadence.ts Outdated
@entelecheia
entelecheia force-pushed the feat/40-s4a-doctor branch 4 times, most recently from e1827df to 9d9cac6 Compare October 24, 2026 14:48
@entelecheia
entelecheia merged commit f2bcf5b into main Oct 24, 2026
2 checks passed
@entelecheia
entelecheia deleted the feat/40-s4a-doctor branch October 24, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants