Skip to content

feat(projectbuild): timestamp tokens for GitHub single-release tags#99

Merged
PrzemyslawKlys merged 1 commit intomainfrom
fix/projectbuild-github-timestamp-tags
Feb 11, 2026
Merged

feat(projectbuild): timestamp tokens for GitHub single-release tags#99
PrzemyslawKlys merged 1 commit intomainfrom
fix/projectbuild-github-timestamp-tags

Conversation

@PrzemyslawKlys
Copy link
Member

Adds date/time token support for GitHub tag/release templates in Invoke-ProjectBuild:\n- {DateTime}, {UtcDateTime} => yyyy.MM.dd.HHmmss\n- {Timestamp}, {UtcTimestamp} => yyyyMMddHHmmss\n\nThis allows Single mode tags to be date-based and unique per run (without depending on GitHubPrimaryProject version).

@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude
Copy link

claude bot commented Feb 11, 2026

PR Review: Timestamp Tokens for GitHub Single-Release Tags

Summary

This PR adds four new date/time tokens ({DateTime}, {UtcDateTime}, {Timestamp}, {UtcTimestamp}) to the GitHub tag/release template system, enabling collision-resistant, time-based tags for single-release mode.


✅ Strengths

  1. Clear Feature Design: The implementation is straightforward and follows the existing pattern for token replacement
  2. Good Documentation: The PR body, code comments, and schema updates are well-written and explain the use case clearly
  3. Consistent Implementation: The new tokens are handled consistently across both PerProject and Single release modes
  4. Schema Updates: JSON schema properly updated with helpful descriptions

🔍 Code Quality Observations

Good Practices

  • Clean, consistent formatting throughout
  • Proper parameter passing through the ApplyTemplate method
  • Documentation updated in multiple locations (docs, XML comments, schema)

Potential Improvements

1. String Replacement Order Issue (Minor)

The ApplyTemplate method uses simple .Replace() calls, which could lead to unexpected behavior with overlapping token names:

.Replace("{Date}", date ?? string.Empty)        // yyyy.MM.dd
.Replace("{DateTime}", dateTime ?? string.Empty) // yyyy.MM.dd.HHmmss

If a template contains {DateTime}, the first replace would turn it into {yyyy.MM.ddTime}, breaking the second replacement.

Recommendation: Replace longer tokens first:

return template
    .Replace("{DateTime}", dateTime ?? string.Empty)
    .Replace("{UtcDateTime}", utcDateTime ?? string.Empty)
    .Replace("{Timestamp}", timestamp ?? string.Empty)
    .Replace("{UtcTimestamp}", utcTimestamp ?? string.Empty)
    .Replace("{Date}", date ?? string.Empty)
    .Replace("{UtcDate}", utcDate ?? string.Empty)
    // ... rest of replacements

2. Potential Race Condition

The timestamp tokens are generated once at the beginning (InvokeProjectBuildCommand.cs:262-269), but in PerProject mode, multiple releases could be created over time if they're slow. This means:

  • All releases in a batch share the same timestamp
  • If the process takes >1 second, the timestamp may not match actual release time

Analysis: This is probably acceptable behavior (consistent timestamps across a batch), but worth documenting if intentional.

3. No Input Validation

The ToString() format strings are hardcoded, which is good, but there's no validation that the DateTime values are valid before formatting.

Risk: Very low - DateTime.Now/UtcNow are always valid, but defensive programming might catch edge cases.


🧪 Test Coverage

Issue: No unit tests found for the ApplyTemplate method or the new token functionality.

Recommendation: Add tests covering:

  • Basic token replacement for all new tokens
  • Edge cases (null/empty templates, missing token values)
  • The overlapping token name scenario mentioned above
  • Template examples from documentation work as expected

Example test structure:

[Fact]
public void ApplyTemplate_WithDateTimeTokens_ReplacesCorrectly()
{
    var result = ApplyTemplate(
        "{DateTime}-{Date}",
        project: "Test",
        version: "1.0.0",
        // ... other params
        dateTime: "2026.02.11.085456",
        date: "2026.02.11"
    );
    
    Assert.Equal("2026.02.11.085456-2026.02.11", result);
}

🔒 Security Assessment

No security concerns identified:

  • No user input is used in format strings (all hardcoded)
  • Token values are generated from trusted DateTime APIs
  • No command injection or path traversal risks
  • Properly sanitized before use in tags/releases

⚡ Performance Considerations

No performance issues:

  • String replacements are O(n) where n is template length
  • DateTime formatting is fast
  • No loops or recursive operations
  • Changes don't impact hot paths

📋 Additional Observations

  1. Whitespace Change: InvokeProjectBuildCommand.Helpers.cs:325 removes a trailing newline - good cleanup
  2. Format Strings: The chosen formats are sensible:
    • yyyy.MM.dd.HHmmss for DateTime (readable, sortable)
    • yyyyMMddHHmmss for Timestamp (compact, sortable, collision-resistant)
  3. Documentation Example: The example "{Repo}-v{UtcTimestamp}" in docs is helpful and demonstrates the intended use case well

📝 Recommendations Summary

Before Merge:

  1. Fix token replacement order in ApplyTemplate to prevent overlapping token issues

Post-Merge (Low Priority):
2. Add unit tests for ApplyTemplate method
3. Consider documenting the "shared timestamp across batch" behavior if intentional
4. Add integration test with example templates from documentation


✅ Overall Assessment

Recommendation: Approve with minor fix

This is a well-implemented feature that solves a real problem (collision-free tags in single-release mode). The code is clean and follows existing patterns. The main issue is the potential token replacement order bug, which should be fixed before merge. Test coverage would be nice to have but isn't blocking given the simplicity of the change.

Quality: 8/10
Risk: Low
Value: High for users needing unique tags per build

@PrzemyslawKlys PrzemyslawKlys merged commit fd4ee0d into main Feb 11, 2026
1 check passed
@PrzemyslawKlys PrzemyslawKlys deleted the fix/projectbuild-github-timestamp-tags branch February 11, 2026 09:13
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