feat(core): support custom and auto-managed created/modified timestamps#1089
feat(core): support custom and auto-managed created/modified timestamps#1089phernandez wants to merge 7 commits into
Conversation
Honor a user-supplied `created`/`modified` value in note frontmatter (ISO 8601, date-only or full datetime, naive values assumed local time) instead of always deriving timestamps from file stat times. Also auto-fill `created`/`modified` into frontmatter on note create/update/edit so they're visible in the file, not just the DB: `created` is preserved once set (by BM or the user) and `modified` is bumped on every BM-initiated write unless the request explicitly supplies its own value. Closes #238. Also addresses #684 (system-managed timestamps), since both issues touch the same frontmatter/timestamp code paths. Co-authored-by: Paul Hernandez <60959+phernandez@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
![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: e915d0c99d
ℹ️ 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".
| created = self._frontmatter_timestamp(metadata, _CREATED_KEYS) | ||
| if created is None: | ||
| created = datetime.fromtimestamp(ctime).astimezone() if ctime else now | ||
|
|
||
| modified = self._frontmatter_timestamp(metadata, _MODIFIED_KEYS) | ||
| if modified is None: | ||
| modified = datetime.fromtimestamp(mtime).astimezone() if mtime else now |
There was a problem hiding this comment.
When notes are written through the v2/MCP path, this parsed timestamp is not what ends up on the entity row: materialization re-indexes the file and index_changed_markdown_file() later updates created_at/updated_at from file_metadata (src/basic_memory/index/local_dependencies.py:515-522). I confirmed through the new write_note integration path that a file containing created: 2024-03-15 returns a created_at at write time, so #238 is still broken for the main API/MCP workflow; carry the parsed markdown timestamps into the final metadata update instead of overwriting them with stat times.
Useful? React with 👍 / 👎.
| frontmatter = _read_frontmatter(test_project, "timestamps/Historical Import.md") | ||
|
|
||
| # BM never overwrites a timestamp the frontmatter already carries. | ||
| assert frontmatter["created"] == "2024-03-15" |
There was a problem hiding this comment.
With PyYAML, the unquoted ISO date scalar in this file is loaded by parse_frontmatter() as a datetime.date, not a string, so this assertion fails even when the file contains the expected text. Running uv run pytest tests/markdown/test_entity_parser.py tests/services/test_entity_service_prepare.py test-int/mcp/test_frontmatter_timestamps_integration.py -q fails here and on the analogous datetime assertion at line 121; compare isoformat() or assert against the raw file text if verbatim preservation is the requirement.
Useful? React with 👍 / 👎.
Local validationCI didn't auto-run on this branch (bot-token push + Both failures are test-assertion bugs, not feature bugs. The feature works — the written file gets
Fix: assert against the parsed Minor real inconsistency worth deciding on: |
Anthropic's OIDC exchange is broken for the pull_request_target event (401 'Invalid OIDC token'; anthropics/claude-code-action#713). The workflow invoked that path whenever a PR body contained '@claude' — and bot-authored PR descriptions carry attribution text ('Generated by the @claude workflow'), so opening #1088/#1089/#1090/#1091 self-triggered the broken path and each failed its 'claude' check with a 401. A normal issue_comment run succeeded with the same secret in between, so the OAuth token is valid and needs no rotation. Remove the pull_request_target trigger and its PR-body '@claude' clause, so attribution text can never be read as a command and no event routes into the broken OIDC path. Explicit '@claude' mentions in issues, issue comments, PR reviews, and PR review comments remain the trigger. Also drop the now-dead pull_request.author_association gate clauses (reviewer identity is covered by sender.author_association) and the pull_request_target-only checkout ref. If automatic PR-open review is wanted later, it should be a dedicated workflow with a fixed prompt, not a PR-body mention. Signed-off-by: phernandez <paul@basicmachines.co>
…matter dates The two integration tests asserted created/modified round-trip as strings, but the code left a user-supplied created as a YAML-parsed date object: it dumped unquoted (created: 2024-03-15) and reparsed to a date, while BM's own modified was an isoformat string that SafeDumper quoted. That split disagreed with the tests and with BM's established convention (#236) of normalizing every frontmatter date to an ISO string. Normalize both timestamps through the existing normalize_frontmatter_value in apply_default_timestamps so they serialize identically (quoted ISO strings), matching how BM writes every other date field. Obsidian still reads them fine; inventing unquoted output for just these two fields would be the inconsistency. Fixes the 2 failing tests in test_frontmatter_timestamps_integration; 30 pass, plus 108 across markdown + write/edit integration with no regressions. Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f705b1873
ℹ️ 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".
| edit_post = frontmatter.Post(remove_frontmatter(markdown_content)) | ||
| edit_post.metadata.update(content_frontmatter) | ||
| now = datetime.now().astimezone() | ||
| apply_default_timestamps( | ||
| edit_post.metadata, | ||
| incoming_metadata={}, | ||
| fallback_created=entity.created_at or now, | ||
| now=now, | ||
| ) |
There was a problem hiding this comment.
For an entity that was indexed from a legacy/plain markdown file without frontmatter, has_frontmatter(markdown_content) is false, so this new timestamp rewrite is skipped and edit_entity_with_content() writes the edited body back without any created/modified frontmatter. That leaves the #684 edit path incomplete for existing frontmatterless notes even though the edit is a BM-initiated write; the timestamp injection needs to also construct frontmatter when none is present.
Useful? React with 👍 / 👎.
#1089 stamps created/modified into every note's frontmatter (#684), which broke 8 existing tests that assert exact note content/metadata. Add a _without_managed_timestamps helper to each affected test file that strips BM's dynamic created/modified frontmatter lines before the exact-content comparison, and assert their presence in test_write_note so the unit layer still documents that write_note emits them. The timestamp behavior itself is covered by test_frontmatter_timestamps_integration.py. Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df954e6909
ℹ️ 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".
| if "created" not in metadata: | ||
| metadata["created"] = fallback_created.isoformat() | ||
| if "modified" not in incoming_metadata: | ||
| metadata["modified"] = now.isoformat() |
There was a problem hiding this comment.
When a BM create/update imports frontmatter that uses the aliases this commit now parses (date/created_at or updated_at), this defaulting treats the canonical created/modified fields as missing and writes the current time instead. Because _frontmatter_timestamp() checks the canonical keys before the aliases, the just-added defaults then win over the imported timestamp, so write_note with date: 2020-01-01 is stored/indexed as created now rather than 2020. Seed canonical fields from the accepted aliases before falling back to now.
Useful? React with 👍 / 👎.
| # Outcome: the frontmatter block written to disk gets modified bumped to "now" (and | ||
| # created filled in if this is the first write to carry one), instead of | ||
| # silently keeping a stale modified value from before the edit. | ||
| edit_post = frontmatter.Post(remove_frontmatter(markdown_content)) |
There was a problem hiding this comment.
For edit_note on any note with frontmatter whose body starts with significant whitespace, this reserialization passes the edited note through remove_frontmatter(), which strips the body. An append to a note whose first body line is an indented code block will write it back without the leading spaces, corrupting content the user did not edit; use a frontmatter split that preserves the body bytes before dump_frontmatter().
Useful? React with 👍 / 👎.
Adversarial review (Codex) caught that the helper was typed str but fed read_note()'s str | dict result, so 'just typecheck' failed with invalid-argument-type at 5 call sites. Accept the union and assert-narrow to str inside the helper (one place, instead of an isinstance guard at every call). Signed-off-by: phernandez <paul@basicmachines.co>
…write paths
Adversarial (Codex) review of the timestamp feature found it only held on the
direct MCP write path; the reindex and cloud paths overwrote the note's
frontmatter timestamps. Per the file-is-source-of-truth rule, the parsed
frontmatter timestamps (file-stat fallback) now win everywhere:
- batch_indexer: the reindex/sync path re-derived created_at/updated_at from raw
file stats via _entity_metadata_updates, clobbering frontmatter timestamps on
every reindex. Markdown now keeps the parsed values from
upsert_entity_from_markdown; they are re-asserted explicitly on the UPDATE
because updated_at's onupdate=now default would otherwise overwrite modified.
- accepted_note_write_runner: the cloud DB-first accepted-write hardcoded
created_at/updated_at to now. Both now derive from the prepared frontmatter
(fallback now), so a cloud note's DB row matches its markdown.
- edit path (entity_service): an edit that explicitly changed created/modified
is now honored instead of always overwritten (was incoming_metadata={}). Also
re-serialize only the frontmatter block and splice the body back rather than
routing it through remove_frontmatter().strip().
- entity_parser: a present-but-unparseable canonical created/modified now fails
fast instead of silently falling back to file stats; the read-only aliases
(date/created_at/updated_at) stay lenient for imported notes.
Regression tests: batch reindex preserves historical frontmatter timestamps;
accepted create/update writes seed DB timestamps from frontmatter. 951 pass, no
regressions.
Findings from cross-model review with anthropics/claude-code-action Codex pass.
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d32ee635cd
ℹ️ 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".
| if isinstance(value, datetime): | ||
| return value | ||
| if isinstance(value, str): | ||
| return datetime.fromisoformat(value) |
There was a problem hiding this comment.
In the DB-first accepted write path, prepared frontmatter has already been parsed by EntityParser.parse_date(), which accepts formats such as Jan 15, 2024 and 2024-03-15 10:00 AM and localizes offsetless values. Re-parsing the same metadata with datetime.fromisoformat() makes a cloud create/update fail for those otherwise valid timestamps (and makes date-only values naive), while the local/indexing path accepts them; use the parsed prepared timestamp or the same parser here to keep cloud and local semantics aligned.
Useful? React with 👍 / 👎.
Adversarial re-review flagged that the _split_frontmatter splice added for body preservation asserts on BOM-prefixed notes: has_frontmatter()/parse_frontmatter() strip the BOM so the branch is entered, but _split_frontmatter() sees the raw BOM and returns None. Body normalization on edit is by-design in BM anyway (every write path strips the body), so revert to remove_frontmatter() (which strips the BOM the same way) and keep only the explicit-timestamp detection (an edit that changed created/modified still wins over the auto-bump). Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed1dba6be
ℹ️ 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".
| # fallback. created_at is intentionally left untouched here — an update must not move it. | ||
| entity.updated_at = _timestamp_from_metadata( |
There was a problem hiding this comment.
For DB-first accepted replaces/edits where the request explicitly changes created in frontmatter, the prepared markdown and entity_metadata now contain the new creation timestamp, but this path intentionally leaves entity.created_at unchanged. The accepted search row is then refreshed from the stale entity value, so cloud/accepted writes can show one created value in the note and a different created_at in search/API results; local upsert does not have this divergence because it reapplies parsed markdown timestamps to both columns.
Useful? React with 👍 / 👎.
…fied Second adversarial (Codex) pass found the timestamp feature's real root cause: the Entity.updated_at column carried onupdate=now, so EVERY row UPDATE re-stamped the note's semantic modified to the wall clock unless the value was explicitly in the SET clause — which repository.update (ORM setattr of an unchanged value) never produces. That clobbered file-authored timestamps at the post-write checksum updates (entity_service create/update/edit), the batch reindex path, and cloud materialization, even though the markdown file kept the right value. Centralized fix: drop onupdate=now from Entity.updated_at (NoteContent's audit column keeps it). updated_at is now only ever set deliberately — from the parsed frontmatter / file mtime on every content write (entity_model_from_markdown and the accepted write) — so incidental bookkeeping updates can no longer overwrite it. File is the source of truth (#238/#684). No migration: onupdate is Python-side ORM behavior, not a DB constraint. Also from the same pass: - materialization writes the physical time to mtime only, preserving the semantic updated_at (flag_modified pins it against any future onupdate). - accepted-write reconciles created_at from frontmatter too (was only updated_at), and localizes naive date-only values to match EntityParser. - apply_default_timestamps treats a null created/modified as missing so it fills the managed default instead of writing YAML null back. - file_bookkeeping_update helper centralizes the checksum writes' intent. Regression tests: create preserves historical frontmatter timestamps through the checksum update; materialization keeps semantic updated_at while recording physical mtime. Full unit suite green (3941 passed) with onupdate removed — nothing relied on the auto-bump that content writes don't already set explicitly. Signed-off-by: phernandez <paul@basicmachines.co>
Closing as supersededAfter tracing the timestamp flow through parsing, local indexing, direct MCP/API writes, DB-first accepted writes, materialization, and the entity model, this PR should not be merged as-is. It combined #238 and #684 before the timestamp ownership and write-policy decisions were fully settled. The successive review fixes then accumulated multiple competing implementations:
CI is green, but those are contract and architecture gaps rather than test-run failures. We are restarting in two deliberate steps:
The useful investigation and regression cases from this branch can inform the replacement work, but the implementation itself should be rebuilt from the clarified contracts. |
1 participant
Implements #238 and #684 in one PR (shared frontmatter/timestamp code path). Generated by the
@claudeworkflow; opened for review — not yet verified by a human.Summary
created/modifiedvalue in frontmatter (ISO 8601, date-only or full datetime; naive values assumed local time) instead of always deriving from file stat times. (Feature Request: Support custom timestamps in note frontmatter #238)created/modifiedinto frontmatter on create/update/edit so they're visible in the file, not just the DB.createdis preserved once set (by user or BM);modifiedbumps on every BM-initiated write unless the request supplies its own. (Auto-sync entity created_at / updated_at timestamps into frontmatter #684)created/modified— the fields the parser already reads (markdown/utils.py), so no parallel scheme was introduced.Review checklist
createdsurvives a subsequent edit (user value wins)Closes #238
Addresses #684
🤖 Generated with Claude Code