Repository navigation
feat(doctor): amem doctor report and sync cadence without a daemon (Refs #40 F15, F16) - #52
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
entelecheia
force-pushed
the
feat/40-s4a-doctor
branch
4 times, most recently
from
October 24, 2026 14:48
e1827df to
9d9cac6
Compare
entelecheia
force-pushed
the
feat/40-s4a-doctor
branch
from
October 24, 2026 15:08
9d9cac6 to
3ac0753
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
agent-mem#40 F15, F16 and AC8.
amem doctor [--json]: one read-only report, nine sections in a fixed order, eachok,attentionorunavailable(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. Newsrc/memory/doctor.ts, adoctorbranch insrc/cli.tsright afterstatus, and a command-table entry.--jsonisamem.doctor/v1.test/doctor-snapshot.test.tspins every key, value type and the fixed vocabulary againsttest/fixtures/doctor-snapshot.json, for a populated and an unused home.amem ingest <cli>at SessionEnd (Codex, Kimi) andsession_shutdown(Pi), and Claude has its hooks. Doctor reads the setup receipts and reports a CLI with no registered hook asmanual, with the command to register it.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.--forceneeds--if-due. State issync-state.json(two times and a failure count, never an error text, mode 0600), stamped by everyamem 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 indocs/contracts.mdwith 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 reportsskippedfor that (its build already proves equality). The brief is measured throughsessionStartBrief({ 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 isunavailable, nevercomplete.test/doctor.test.tshashes every file of a populated home before and after the FIRST run (no warm-up),-walincluded. The one thing not compared is a-shmfile'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 workmutation-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.dbwas absent: the scratch base replays the log. (4)sync --if-due --jsonstamped 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 isskippedand 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
ghvisibility lookup (its own 15 s) or the cleanupmerge --abort(up to 60 s), which deliberately ignore the stop so the repository is left clean. Both are indocs/contracts.md.Also changed:
src/memory/sync.ts(the git spawn, only when a stop signal is passed) andptyArgvmoved fromtest/review-pending.test.tstotest/fakes/pty.tsso two test files share it.Review round 3
sync --if-dueruns 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 useon, notonce, 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,oncefails 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-waland-shmfiles on a read-only open; the test now removes both first and normalises only those, which is explained indocs/contracts.md. Stated: SIGKILL of amem itself cannot be handled by anyone, so a git already running then has nothing to stop it;--if-duedoes not widen that old hazard of any killed sync.One existing test changed on purpose
test/completion.test.tslists the flags a raw hub-shared command may not accept.sync --if-due --forceare handled insrc/cli.tsbefore the raw command (which never sees them: checked), so they join the named exemptions, likesearch --all-projects.Check
tsc clean, boundary OK (73 files),
npm pack --dry-run81 files,bun test --timeout 200001300 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,--jsonfailure 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 theunavailableguard changed nothing visible because the work log was absent, so I added a brief-level test and the guard now fails it when it claimscomplete.Refs #40