Skip to content

fix(header): preserve existing @Last modified by identity unless forced - #25

Merged
Shinrai merged 5 commits into
nextfrom
fix/preserve-last-modified-identity
Aug 9, 2026
Merged

fix(header): preserve existing @Last modified by identity unless forced#25
Shinrai merged 5 commits into
nextfrom
fix/preserve-last-modified-identity

Conversation

@cldmv-bot

@cldmv-bot cldmv-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

🚀 What's Changed

💥 Breaking Changes

No breaking changes

✨ Features

No new features

🐛 Bug Fixes

📦 Dependencies

No dependency updates

🔧 Other Changes

👥 Contributors

The comparison header always stamped the current runner's git identity into
@last modified by, so any machine whose git config user.name differed from
what was recorded in existing headers flagged nearly every file as changed
purely from an identity mismatch (1551/1565 in one observed run), even with
zero real content change.

Add extractHeaderLastModifiedIdentity() to parse the existing @last modified
by line and preserve it in the comparison header unless the new
forceLastModifiedAuthorUpdate option (--force-last-modified-author-update) is
set, mirroring how @Author/@Email is already gated behind forceAuthorUpdate.
Uses a dedicated flag rather than reusing forceAuthorUpdate since the two
identities are independently meaningful to refresh.

Closes #24
@cldmv-bot cldmv-bot Bot added ! fix → next v4 flow: fix contributor PR targeting the next integration branch area: core Touches core library / runtime source code area: tests Touches test files, fixtures, or test infrastructure type: documentation Relates to docs, README updates, guides, or inline code comments labels Aug 9, 2026
@Shinrai
Shinrai requested a lite review from Copilot August 9, 2026 04:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts the header-rewriting engine so an existing header’s @Last modified by identity is preserved (and does not trigger a rewrite) unless explicitly forced, and adds a new CLI/API option to force updating the last-modified identity.

Changes:

  • Add forceLastModifiedAuthorUpdate option to preserve existing @Last modified by identity by default, with an override to force updating it.
  • Extend CLI parsing/help text and README documentation to expose --force-last-modified-author-update.
  • Add/adjust Vitest coverage to validate preservation vs forced update behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/core-edge.test.vitest.mjs Adds a no-op preservation test and a force-update test for last-modified identity behavior.
tests/cli.test.vitest.mjs Adds coverage for parsing --force-last-modified-author-update.
src/core/fix-headers.mjs Implements extraction/preservation of existing @Last modified by identity unless forced.
src/cli.mjs Documents and parses the new --force-last-modified-author-update flag.
README.md Documents the new option and clarifies default preservation behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/core-edge.test.vitest.mjs Outdated
…ar dynamic

The fixture header hard-coded a 2013-2026 copyright range, which mismatches
the tool's default (current year) regardless of forceLastModifiedAuthorUpdate
— the test would still report filesUpdated: 1 even if the force flag were a
no-op, since the copyright-line mismatch alone forces the rewrite. Match the
sibling preservation test's pattern of a dynamic current-year range so the
force flag is the only reason the header changes.

Addresses review comment on PR #25.
@Shinrai

Shinrai commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Resolves #24 — closed when this ships to the default branch.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/core/fix-headers.mjs:308

  • forceLastModifiedAuthorUpdate only affects the comparison header (used to decide whether a file needs an update). When needsUpdate is true for any other reason, the subsequent buildHeader(...) call still hard-codes lastModifiedByName/lastModifiedByEmail to the current detected author, so @Last modified by will change even when the new flag is not set. This appears to contradict the PR intent/docs of preserving the existing last-modified identity unless forcing it; consider applying the same conditional (shouldForceLastModifiedAuthorUpdate ? fileMetadata : existingLastModifiedIdentity || fileMetadata) to the needsUpdate ? buildHeader(...) branch as well.
			lastModifiedByName: shouldForceLastModifiedAuthorUpdate
				? fileMetadata.authorName
				: existingLastModifiedIdentity.authorName || fileMetadata.authorName,
			lastModifiedByEmail: shouldForceLastModifiedAuthorUpdate
				? fileMetadata.authorEmail

…fied identity

forceLastModifiedAuthorUpdate only gated the comparison header used to decide
whether a rewrite was needed. Once any other reason triggered needsUpdate
(e.g. forceAuthorUpdate forcing createdBy to differ, or a malformed/missing
@last modified time repair), the actual rewritten header unconditionally
stamped the current runner's identity into @last modified by, bypassing the
flag entirely and defeating the point of splitting it from forceAuthorUpdate.

Apply the same preserve-unless-forced gate to the rewrite branch. @last
modified time still always refreshes to now on any real rewrite — only the
identity is gated.

Addresses suppressed review finding on PR #25.
@Shinrai

Shinrai commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Addressed the suppressed finding from the latest review (src/core/fix-headers.mjs:308): forceLastModifiedAuthorUpdate only gated the comparison header used to decide whether a rewrite was needed. Once any other reason triggered needsUpdate — e.g. forceAuthorUpdate forcing createdBy to differ, or a malformed/missing @Last modified time repair — the rewritten header unconditionally stamped the current runner's identity into @Last modified by, bypassing the flag and defeating the point of splitting it from forceAuthorUpdate.

Confirmed by reproduction: forceAuthorUpdate: true alone (no forceLastModifiedAuthorUpdate) was overwriting @Last modified by to the current identity instead of preserving it.

Fixed in e067f9d — applied the same preserve-unless-forced gate to the rewrite branch. @Last modified time still always refreshes to now on any real rewrite; only the identity is gated. Added a regression test (does not cascade a forced author update into the last-modified identity of an unrelated rewrite) and strengthened the existing force-author-update test with the missing @Last modified by assertion.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/core/fix-headers.mjs:33

  • FixHeadersOptions now documents forceLastModifiedAuthorUpdate, but the published TypeScript declarations still don’t include this option (e.g. types/src/core/fix-headers.d.mts only has forceAuthorUpdate / useGpgSignerAuthor). This will prevent TS consumers from using the new flag without type errors; please update the declarations (or the generation step) to keep the public API in sync.
 *  configFile?: string,
 *  sampleOutput?: boolean,
 *  forceAuthorUpdate?: boolean,
 *  forceLastModifiedAuthorUpdate?: boolean,
 *  useGpgSignerAuthor?: boolean,

…Update

The published TypeScript declarations were out of sync with the JSDoc source
after adding forceLastModifiedAuthorUpdate to FixHeadersOptions, which would
have produced type errors for TS consumers using the new flag. Regenerated
via the project's own types:build script and kept only the line this PR's
change touches — the regeneration also surfaced unrelated pre-existing drift
in constants.d.mts, file-discovery.d.mts, and detectors/index.d.mts from
other features already merged into next, which is out of scope here and left
untouched.

Addresses suppressed review finding on PR #25.
@Shinrai

Shinrai commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Addressed the suppressed finding from the latest review (src/core/fix-headers.mjs:33): the FixHeadersOptions JSDoc documented forceLastModifiedAuthorUpdate, but the committed TypeScript declarations (types/src/core/fix-headers.d.mts) hadn't been regenerated, so TS consumers would hit a type error using the new flag.

Fixed in 8a2e4d8 — regenerated via npm run types:build and kept only the line this PR's change touches. The regeneration also surfaced unrelated pre-existing drift in types/src/constants.d.mts, types/src/core/file-discovery.d.mts, and types/src/detectors/index.d.mts (stale declarations for the .gitignore support / root-anchor ignores feature already merged into next) — that's out of scope for this PR so I reverted those files and left them as-is. Worth a separate follow-up to regenerate types/ cleanly against current next.

…ubset

The previous commit manually stripped the gitignore-related lines out of the
tsc-regenerated declarations to keep the diff scoped to this PR's own change.
That was wrong: declaration output is fully deterministic from source, so
trimming it just means the file ships already stale the moment this PR
merges (missing gitignore, which this branch's own source already declares).
Regenerate the full, untrimmed output via npm run types:build instead —
picking up the pre-existing gitignore/root-anchor-ignores (74c34da)
declaration drift alongside this PR's forceLastModifiedAuthorUpdate addition,
so next isn't left stale in either direction regardless of merge order.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@Shinrai
Shinrai merged commit c4444e8 into next Aug 9, 2026
24 checks passed
@cldmv-bot
cldmv-bot Bot deleted the fix/preserve-last-modified-identity branch August 9, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: core Touches core library / runtime source code area: tests Touches test files, fixtures, or test infrastructure ! fix → next v4 flow: fix contributor PR targeting the next integration branch type: documentation Relates to docs, README updates, guides, or inline code comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants