Implement contact-card YAML front matter parser/serializer#588
Implement contact-card YAML front matter parser/serializer#588Chris0Jeky merged 8 commits intomainfrom
Conversation
Defines structured contact fields (name, email, handles, tags, status, cadence, etc.) that are stored as YAML front matter in card descriptions.
Static utility that extracts YAML front matter (delimited by ---) from card descriptions into ContactCardFrontMatter, serializes back with round-trip stability, and returns explicit validation errors for malformed input rather than throwing.
Covers round-trip stability, no front matter, empty/null input, malformed YAML, missing closing delimiter, validation errors (type, tier, status), Windows line endings, Unicode, special characters, unknown fields, trailing whitespace on delimiters, and serializer null-omission and naming conventions.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces a YAML front matter parser and serializer for contact cards, enabling structured metadata storage within card descriptions. It includes the ContactCardFrontMatter DTO, the ContactCardYamlParser service, and a comprehensive suite of unit tests. The feedback focuses on improving performance by caching static resources like serializers and validation sets, enhancing data integrity through date format validation, and simplifying test assertions using object equivalency.
| errors.Add($"Invalid status '{fm.Status}'. Expected one of: cold, warm, active, referral, interviewing, closed."); | ||
| } | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
The Validate method is missing validation for LastTouchAt and NextTouchAt properties. This could allow invalid date formats to be saved, potentially causing parsing errors later. Please add validation to ensure these fields, if present, are valid ISO 8601 date strings (e.g., "YYYY-MM-DD").
You can use DateOnly.TryParse() for this before returning the errors. For example:
if (!string.IsNullOrEmpty(fm.LastTouchAt) && !DateOnly.TryParse(fm.LastTouchAt, out _))
{
errors.Add($"Invalid last_touch_at format '{fm.LastTouchAt}'. Expected ISO 8601 date (YYYY-MM-DD).");
}
// and similarly for NextTouchAt| var validTiers = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "A", "B", "C" }; | ||
| if (!string.IsNullOrEmpty(fm.RelationshipTier) && !validTiers.Contains(fm.RelationshipTier)) | ||
| { | ||
| errors.Add($"Invalid relationship_tier '{fm.RelationshipTier}'. Expected one of: A, B, C."); | ||
| } | ||
|
|
||
| var validStatuses = new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| "cold", "warm", "active", "referral", "interviewing", "closed" | ||
| }; | ||
| if (!string.IsNullOrEmpty(fm.Status) && !validStatuses.Contains(fm.Status)) | ||
| { | ||
| errors.Add($"Invalid status '{fm.Status}'. Expected one of: cold, warm, active, referral, interviewing, closed."); | ||
| } |
There was a problem hiding this comment.
The validTiers and validStatuses HashSets are re-created on every call to Validate. For better performance and to adhere to best practices, these should be defined once as static readonly fields within the ContactCardYamlParser class.
You can add these fields to ContactCardYamlParser:
private static readonly HashSet<string> ValidTiers = new(StringComparer.OrdinalIgnoreCase) { "A", "B", "C" };
private static readonly HashSet<string> ValidStatuses = new(StringComparer.OrdinalIgnoreCase)
{
"cold", "warm", "active", "referral", "interviewing", "closed"
};Then, the validation logic can be simplified as suggested.
if (!string.IsNullOrEmpty(fm.RelationshipTier) && !ValidTiers.Contains(fm.RelationshipTier))
{
errors.Add($"Invalid relationship_tier '{fm.RelationshipTier}'. Expected one of: A, B, C.");
}
if (!string.IsNullOrEmpty(fm.Status) && !ValidStatuses.Contains(fm.Status))
{
errors.Add($"Invalid status '{fm.Status}'. Expected one of: cold, warm, active, referral, interviewing, closed.");
}| private static IDeserializer BuildDeserializer() | ||
| { | ||
| return new DeserializerBuilder() | ||
| .WithNamingConvention(UnderscoredNamingConvention.Instance) | ||
| .IgnoreUnmatchedProperties() | ||
| .Build(); | ||
| } | ||
|
|
||
| private static ISerializer BuildSerializer() | ||
| { | ||
| return new SerializerBuilder() | ||
| .WithNamingConvention(UnderscoredNamingConvention.Instance) | ||
| .ConfigureDefaultValuesHandling(DefaultValuesHandling.OmitNull) | ||
| .Build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The IDeserializer and ISerializer instances are built on every call to Parse and Serialize respectively. Since their configuration is constant, they can be created once and stored in static readonly fields for improved performance.
You can define them at the class level:
private static readonly IDeserializer Deserializer = new DeserializerBuilder()
.WithNamingConvention(UnderscoredNamingConvention.Instance)
.IgnoreUnmatchedProperties()
.Build();
private static readonly ISerializer Serializer = new SerializerBuilder()
.WithNamingConvention(UnderscoredNamingConvention.Instance)
.ConfigureDefaultValuesHandling(DefaultValuesHandling.OmitNull)
.Build();Then, you can use these fields directly in Parse and Serialize and remove the BuildDeserializer() and BuildSerializer() methods.
| result.Errors.Should().BeEmpty(); | ||
| result.FrontMatter.Should().NotBeNull(); | ||
| result.FrontMatter!.Type.Should().Be("contact"); | ||
| result.FrontMatter.DisplayName.Should().Be("Jane Doe"); | ||
| result.FrontMatter.RelationshipTier.Should().Be("A"); | ||
| result.FrontMatter.Company.Should().Be("Google"); | ||
| result.FrontMatter.Role.Should().Be("SRE"); | ||
| result.FrontMatter.LocationTz.Should().Be("Europe/London"); | ||
| result.FrontMatter.Handles.Should().ContainKey("email").WhoseValue.Should().Be("jane@example.com"); | ||
| result.FrontMatter.Tags.Should().BeEquivalentTo(new[] { "google", "platform", "referral-target" }); | ||
| result.FrontMatter.Source.Should().Be("GE colleague"); | ||
| result.FrontMatter.Status.Should().Be("warm"); | ||
| result.FrontMatter.CadenceId.Should().Be("warm-3-7-21"); | ||
| result.FrontMatter.LastTouchAt.Should().Be("2026-02-20"); | ||
| result.FrontMatter.NextTouchAt.Should().Be("2026-02-27"); | ||
| result.FrontMatter.NotesPrivate.Should().Be("Met at X; cares about reliability; likes concise messages."); | ||
| result.Body.Should().Be(body); |
There was a problem hiding this comment.
Instead of asserting each property individually, you can use FluentAssertions' BeEquivalentTo to compare the deserialized FrontMatter object with the original one. This makes the test more concise and easier to maintain.
result.Errors.Should().BeEmpty();
result.FrontMatter.Should().BeEquivalentTo(original);
result.Body.Should().Be(body);| result.Errors.Should().BeEmpty(); | ||
| result.FrontMatter.Should().NotBeNull(); | ||
| result.FrontMatter!.DisplayName.Should().Be("Minimal User"); | ||
| result.FrontMatter.Type.Should().Be("contact"); | ||
| result.Body.Should().BeEmpty(); |
There was a problem hiding this comment.
Avoids rebuilding YamlDotNet builders and HashSet allocations on every Parse/Serialize/Validate call. Both ISerializer and IDeserializer are thread-safe after construction.
Adversarial Self-ReviewYAML injection / security
Encoding
Round-trip safety
Edge cases covered
Issues found and fixed
Remaining notes (not bugs)
|
…ch fields - Resolve conflicts with main - Add ISO 8601 date validation for last_touch_at and next_touch_at fields (Gemini HIGH) - Add tests for date format validation
Fixes appliedMerge conflicts resolved — rebased/merged with current main. Gemini HIGH — Date validation added (
Gemini MEDIUMs — Static caching (from self-review commit ba73235): already applied — Gemini MEDIUMs — Test simplification with |
…ep YamlDotNet Keep Microsoft.IdentityModel.Tokens and System.IdentityModel.Tokens.Jwt at 8.17.0 from main (security upgrade) in both Application and Infrastructure projects, while retaining YamlDotNet 16.3.0 added by this PR.
Merge conflict resolved
The prior "Resolve merge conflicts" commit had left both projects at Build: succeeded (0 warnings, 0 errors). ContactCardYamlParser tests: 41 passed. |
Summary
ContactCardFrontMatterDTO modeling structured contact fields (name, handles, tags, status, cadence, etc.) stored as YAML front matter in card descriptions for the card-first Outreach CRM (issue OUT-02: Implement contact-card YAML front matter parser/serializer contract #264, Wave 2 foundation)ContactCardYamlParserstatic utility withParseandSerializemethods: extracts----delimited YAML front matter, deserializes via YamlDotNet with underscore naming convention, validates type/tier/status enums, and returns explicit error lists instead of throwingParse(Serialize(fm, body))produces identical outputCloses #264
Test plan
ContactCardYamlParserTestspassSerialize -> Parse -> Serializestability