Skip to content

write_card: treat a blank history_entry as absent on UPDATE and NOOP - #207

Merged
smixs merged 5 commits into
mainfrom
iva/fix-write-card-empty-history
Aug 27, 2026
Merged

write_card: treat a blank history_entry as absent on UPDATE and NOOP#207
smixs merged 5 commits into
mainfrom
iva/fix-write-card-empty-history

Conversation

@smixs

@smixs smixs commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Blank history_entry on UPDATE/NOOP is treated as absent (same trim as SUPERSEDE). A non-blank entry is still refused. A rejected NOOP leaves the card byte-identical.

Reproduced on main with a failing test before the fix. Related to #205: the existing PR does not assert the rejected NOOP card is unchanged.

Summary by CodeRabbit

  • Bug Fixes

    • Empty or whitespace-only history entries are now treated as omitted for update and no-op actions.
    • Non-empty history entries remain validated correctly, preventing invalid card changes.
    • Superseding a card with a valid history entry now reliably archives the previous fact.
  • Documentation

    • Clarified that empty history entries are equivalent to leaving the field unspecified.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

Next included review available in 7 minutes.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 15f25425-87f5-48cc-9d29-3a9ad555acb2

📥 Commits

Reviewing files that changed from the base of the PR and between 273aa5e and c2de99f.

📒 Files selected for processing (1)
  • scripts/write-card.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 965d2edd-c23b-455d-aadc-200294b54e1b

📥 Commits

Reviewing files that changed from the base of the PR and between 70d3752 and 273aa5e.

📒 Files selected for processing (2)
  • agent/tools/write_card.ts
  • scripts/write-card.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • agent/tools/write_card.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The write-card tool treats empty and whitespace-only history_entry values as absent for UPDATE and NOOP. ADD preserves raw input for merge handling. Tests cover accepted, rejected, unchanged, and SUPERSEDE cases.

Changes

history_entry handling

Layer / File(s) Summary
Normalize and validate history_entry
agent/tools/write_card.ts
The tool trims history_entry for presence checks. UPDATE and NOOP accept empty values as omitted and reject non-empty values under their existing rules.
Route values and verify operations
agent/tools/write_card.ts, scripts/write-card.test.ts
ADD passes the raw value to mergeCard. Tests cover whitespace-only values, rejected non-empty values, unchanged files, and successful SUPERSEDE history creation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 273aa

The change makes blank history entries behave as absent for UPDATE and NOOP while preserving rejection of non-blank entries and unchanged-card behavior; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: treating blank history_entry values as absent for UPDATE and NOOP.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@smixs smixs left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verified. On main the UPDATE/NOOP guard is history_entry !== undefined, so "" / whitespace is a forged-archive refusal and a model that fills every schema field loops. The tool-boundary trim matches how SUPERSEDE already reads the field; a non-blank entry is still refused; ADD still receives the raw value for the journaled noise-drop. Rejected NOOP leaves the card byte-identical — that assertion is why this should land instead of #205 (same production diff, weaker test).

Philosophy: this is deterministic input normalization at the tool contract, not a new mechanism, not a skill. Design closes the class (blank ≡ absent); the test pins it.

Unproven: the live gpt-5.6-luna 300–500-refusal loop (model behaviour). Unicode whitespace is covered by JS trim(). Ready.

@smixs
smixs force-pushed the iva/fix-write-card-empty-history branch from 70d3752 to 0c63dd9 Compare August 27, 2026 01:43
@smixs
smixs merged commit 39d9e5d into main Aug 27, 2026
1 check passed
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.

1 participant