Strips `version` and `timeUpdated` fields from filters.json and filters.js by mu-hun · Pull Request #1226 · AdguardTeam/FiltersRegistry

mu-hun · 2026-08-14T09:49:20Z

Extends --strip-generated-meta (previously .txt only) to also drop these noisy fields from compiled metadata files. Closes #1208.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Extends the --strip-generated-meta build option to also remove noisy generated fields (version, timeUpdated) from compiled metadata files (filters.json, filters.js) to reduce irrelevant diffs during build comparisons (closes #1208).

Changes:

Add stripping of version/timeUpdated from filters.json and filters.js during the strip-generated-meta pass.
Update build output comparison messaging and developer documentation to reflect the new behavior.
Add tests covering stripping behavior for filters.json/filters.js.

Reviewed changes

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

Show a summary per file

File	Description
scripts/compare-build-output-against-master.sh	Updates report text to treat metadata diffs as meaningful rather than expected noise.
scripts/build/strip-generated-meta.ts	Adds JSON metadata stripping logic and expands the directory walk to target `filters.json`/`filters.js`.
scripts/build/build.js	Updates comment describing what `--strip-generated-meta` now removes.
scripts/build/tests/strip-generated-meta.test.ts	Adds unit tests for stripping `version`/`timeUpdated` from metadata files.
DEVELOPMENT.md	Documents expanded behavior of `--strip-generated-meta`.

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

mu-hun · 2026-08-19T06:34:40Z

            if (entry.name.endsWith(TXT_FILE_EXTENSION)) {
                return (await stripGeneratedMeta(fullPath)) ? 1 : 0;
            }
+            if (METADATA_FILE_NAMES.includes(entry.name)) {
+                return (await stripMetaFromMetadataFile(fullPath)) ? 1 : 0;
+            }
            return 0;


This review doesn't seem important or has a lower priority because all the compiled .txt filters are under the filters/ directories, except for the local_script_rules.txt file located under each platform directory.

105th

AI Code Review (`open-code-review`)

Findings from an automated review of this PR (extends --strip-generated-meta to remove version/timeUpdated from filters.json/filters.js, closing #1208).

Summary: 5 findings — 4 medium, 1 low. No blocking (high) issues found. Main themes: (1) robustness of the new stripMetaFromMetadataFile against non-JSON/null inputs, (2) avoiding the exact formatting noise this PR aims to eliminate, and (3) a documentation label in compare-build-output-against-master.sh that will mislead for this PR's own validation run.

Details in the inline comments below.

105th · 2026-08-19T12:42:08Z

  echo ""
  echo "${C_BOLD}--- Metadata Files (informational) ---${C_RESET}"
-  echo "filters.json/filters.js diffs: $meta_diffs  (version counter noise, not a regression)"
+  echo "filters.json/filters.js diffs: $meta_diffs (a nonzero count here signals a metadata change)"


The relabeled message claims a nonzero count always signals a true metadata change, but that holds only once master is rebuilt with this PR's strip logic. In this script the master worktree is built with its own pre-PR build.js/strip-generated-meta.ts, which strips only .txt files and still writes version/timeUpdated into filters.json/filters.js. So during this PR's own validation run, every filters.json/filters.js will differ purely from the strip asymmetry (master retains the fields, the feature branch deletes them) — producing a nonzero meta count that is exactly the version noise issue #1208 aims to eliminate, not a metadata change.

Recommendation: cover this transitional case, e.g. note that a nonzero count may be version/timeUpdated noise until master includes the metadata strip.

As I understand it, the new specific content changes are only from $CHANGED_BRANCH and are not included in $BASED_BRANCH. Is my understanding correct? However, I've already added a step to sync the filters/ directory using git checkout $MASTER_SHA -- filters/.

FiltersRegistry/scripts/compare-build-output-against-master.sh

Line 385 in b5b1491

git -C "$CHANGED_WORK_TREE" checkout "$MASTER_SHA" -- filters/; then

This overwrites the changed worktree's filters/ (including revision.json) with master's exact content, so both worktrees compile from identical source.

…`filter` prop #1226 (comment)

#1226 (comment)

105th

Reviewed the full diff (4 files; DEVELOPMENT.md skipped as docs). The stripping logic itself looks correct — metadata-file matching, the record-array guard, and the modified-file counting all check out, and the tab-indented serialization matches what @adguard/filters-compiler writes (both files are plain JSON, no trailing newline), so stripping is self-consistent for the compare workflow.

One blocking concern (a dead guard + a lost try/catch that can abort the build with a file-path-less error) and two test-coverage gaps below.

Minor (non-blocking): in the "clean file" case (strip-generated-meta.test.ts:171) consider asserting the file content stays byte-identical to pin the deliberate do-not-rewrite behavior.

#1226 (comment)

105th

Code Review Summary

Ran an independent AI code review (ocr / open-code-review) on the full diff (5 files, +245/−19).

Verdict: No actionable issues found. The implementation is solid.

What this PR does

Extends --strip-generated-meta to also remove version/timeUpdated fields from filters.json/filters.js metadata files, in addition to the existing .txt meta-line stripping. Goal: clean, reproducible build-output diffs.

Verification highlights

filters.js is strict JSON: Confirmed against the actual committed platforms/cli/filters.js and platforms/cli/filters.json — both parse cleanly with JSON.parse (tab-indented, LF, no CRLF, contains real version/timeUpdated fields). The compiler emits strict JSON with a .js extension, not a JS module, so JSON.parse is safe.
Robustness guards are correct: isRecordArray properly excludes null and arrays via !Array.isArray(item); the parsed value is checked for non-null/non-array before accessing .filters; try/catch around JSON.parse rethrows with a contextual file-path message; missing/null/non-array/filters and bad entries return 0 gracefully.
Formatting preserved: JSON.stringify(data, null, '\t') matches the compiler's own tab indentation, and the trailing-newline is preserved via content.endsWith('\n') — no spurious full-file reformatting.
Test coverage is thorough: strips success, indentation/trailing-newline preservation, no-op on clean files, throw-on-invalid-JSON (file left untouched), missing/null/non-array filters, null/array entries, and empty filters array.

Approved.

mu-hun · 2026-08-24T05:34:08Z

Thanks for your approval! I'd suggest just holding the current state until the parent PR is merged, rather than merging this into another PR. Since I'm following a stacked PR workflow (https://www.stacking.dev/), I'd like to keep each PR as a single commit when merging into master.

…`filter` prop #1226 (comment)

#1226 (comment)

mu-hun · 2026-08-24T08:34:35Z

Note on the filters.json/filters.js diffs: 32 count from compare-build-output-against-master.sh:

--- Metadata Files (informational) ---
filters.json/filters.js diffs: 32 (a nonzero count here signals a metadata change)

This is expected, not a regression. Master's strip-generated-meta.ts doesn't have stripMetaFromMetadataFile / GENERATED_META_FIELDS / METADATA_FILE_NAMES — it only strips generated lines from .txt filter files and never touches filters.json/filters.js. That capability is added in this PR, so master's build output still carries version/timeUpdated, while this branch's output has them stripped:

diff temp/platforms_master_build/mac/filters.json temp/platforms_changed_build/mac/filters.json
49,50d48
-                       "version": "2.1.8.22",
-                       "timeUpdated": "2026-08-21T19:13:03+0000",
64,65d61

Once this feature is merged to master, the metadata-diff count against master will drop to 0 for unrelated branches. Until then, any branch compared against current master will show a nonzero count here. That's a known baseline mismatch, not a signal to investigate.

Alex-302 · 2026-08-25T09:42:57Z

Some of the phrases in the script interface are a little long.

Found build output from a previous run (feature/#1208 vs master, plain) — generate the report from it now instead of rebuilding?

=> Output directory is not empty ($CHANGED_BRANCH vs $BASED_BRANCH, $MODE_LABEL). Reuse it, or [delete and] rebuild? Without [delete and] also clear I think.
Current default is No. I'd prefer to change to Yes, since we may not be sure exactly what is in the old files.

Ignoring previous build output, rebuilding

old files removed or not? If removed => Removing previous build output, rebuilding

Step 3/9: Cleanup preference
Remove worktrees and platforms_*_build/ when done?

=> Remove build output when done?

Step 4/9: Set up worktrees
[master] Existing worktree found at C:/Work/AdGuard/Repos/AdGuard/Main/FiltersRegistry/temp/reg-master-build — reuse it instead of recreate from scratch?

Long line, more readable as two

=> [master] Existing worktree found at C:/Work/AdGuard/Repos/AdGuard/Main/FiltersRegistry/temp/reg-master-build
=> Reuse it instead of rebuilding?

mu-hun · 2026-08-25T10:54:05Z

Since here is nighttime, I will apply your comment tomorrow morning. 💤

Was three: "stripped generated meta fields", "stripped metadata from N file(s)", and build.js's "Stripped generated meta from N file(s).". Align the two in this file on "stripped generated meta[ from N file(s)]". #1226 (comment)

The only module-level list without immutability typing. `readonly string[]` (not `as const`, which would break `.includes(entry.name)`). #1226 (comment)

"preserves tab indentation and trailing-newline from filters.json and filters.js" only wrote filters.json — writeMetadataFiles fell back to its `['filters.json']` default. Pass both names and assert each output is byte-identical to the expected serialization. #1226 (comment)

- rename the `testFile` fixture to `testFilePath`; it holds a full path, like its `oldFilePath` / `newFilePath` siblings - fold the standalone empty-`filters`-array case into the adjacent table-driven missing/null/non-array test so the skip contract lives in one place #1226 (comment) #1226 (comment)

maximtop

Independently validated findings from a multi-agent review. Only confirmed, non-duplicate issues are included.

mu-hun · 2026-09-07T04:22:20Z

Only confirmed, non-duplicate issues are included.

What does the above mean? I didn't see any specific feedback in the code review to go off of.

maximtop · 2026-09-07T06:05:47Z

Only confirmed, non-duplicate issues are included.

What does the above mean? I didn't see any specific feedback in the code review to go off of.

never mind, the agent went rogue

mu-hun · 2026-09-07T07:51:54Z

I see. I'm waiting your additional reviews and approval 👀

maximtop

No further comments from me. Ready to merge.

Alex-302 · 2026-09-09T13:12:27Z

Ready to merge

after #1211?

mu-hun · 2026-09-11T04:11:54Z

👀 Right. This PR will merge after down(parent) stack PR merged.

Squashed from PR #1226 review-fixup commits: b59c6c4 1e32924 89b3595 e56cb35 36802bf defaee8 30913ec ecc7ce8 564ecf6 6acf8e8 6a22bb0 c85a00f 9ef7734 17427e4

Squashed from PR #1226 review-fixup commits: e4d91aa 471fc84 922c346 c2b206f 2dbc2d2 8a63c55

…ndant type guard Squashed from PR #1226 review-fixup commits: 33bf686 e65cd82

`yarn strip-generated-meta` with no argument walked the tracked `platforms/` tree and rewrote files in place. Since this PR the pass also strips version/timeUpdated from the 32 committed filters.json/filters.js, so a stray `git add -A` after a bare run would ship metadata the products read, with nothing in CI to catch it. Drop the default: the CLI now needs an explicit target and errors on a missing dir; the single-dir path replaces the old Promise.all/filter machinery. The --strip-generated-meta build flag is unaffected — build.js passes its own root. Document the standalone command in DEVELOPMENT.md, fix the stale "metadata lines" wording in the Command Compatibility list, and note the required <dir> in the AGENTS.md command table. #1226 (comment) #1226 (comment) #1226 (comment)

…eporting Squashed from PR #1226 review-fixup commits: fbefade 67e6042 fa9e30a ea40dfd 598315a

A `%` in a filename prints an invalid format character error and truncates the entry, and backslashes get re-interpreted. printf '%b\n' "$diff_list" did the same \n expansion with the data as an argument. Squashed from PR #1226 review-fixup commits: 98315a702836c7a5643c9a8f6851deaad6cb8ef #1226 (comment)

mu-hun · 2026-09-23T06:02:41Z

This PR includes commits from #1212 due to a rebase a few weeks ago. I've squashed the commits by catalog, so please take another look when you have a chance.

Note: I discovered an issue where the metadata verdict is only FAILED when running in normal build mode.

--- Verdict ---
✗ FAIL
  34 metadata file(s) differ

Ensure metadata (groups/tags/locales) is available during cached builds. Previously, the compiler skipped metadata generation because the source files weren't present, leading to false PASS results in diff checks. Fix: Copy groups/tags/locales alongside filters_cached/ before cached builds and clean them up afterward.

mu-hun · 2026-09-23T07:27:28Z

Fixed the --use-cache bug (filters.json/filters.js were silently omitted, see commit above).

One thing still shows up in the verdict — the "N metadata file(s) differ" — but that's not new, it's the same transitional mismatch I flagged earlier in #1226 (comment): master doesn't have this PR's filters.json/filters.js stripping yet, so it still carries version/timeUpdated.

To reproduce cleanly without that noise, run git branch -f master HEAD locally first (simulates post-merge state), then re-run compare-build-output.

105th

approving — the strip logic looks right, and the metadata gate asymmetry is already tracked in the thread above

mu-hun self-assigned this Aug 14, 2026

mu-hun requested a balanced review from Copilot August 14, 2026 09:49

Copilot AI reviewed Aug 14, 2026

View reviewed changes

mu-hun requested review from 105th, Alex-302, maximtop and slvvko and removed request for maximtop August 19, 2026 06:07

105th reviewed Aug 19, 2026

View reviewed changes

105th approved these changes Aug 19, 2026

View reviewed changes

mu-hun added a commit that referenced this pull request Aug 20, 2026

Verify the parsed value is a non-null, non-array before accessing to …

539cf20

…`filter` prop #1226 (comment)

mu-hun added a commit that referenced this pull request Aug 20, 2026

Fix type guard to check is not array

6d7c9e2

#1226 (comment)

mu-hun added a commit that referenced this pull request Aug 20, 2026

Preserve the original trailing newline if exist

4945cee

#1226 (comment)

mu-hun requested a review from 105th August 20, 2026 07:15

105th requested changes Aug 20, 2026

View reviewed changes

Comment thread scripts/build/strip-generated-meta.ts Outdated

Comment thread scripts/build/__tests__/strip-generated-meta.test.ts

Comment thread scripts/build/__tests__/strip-generated-meta.test.ts Outdated

mu-hun added a commit that referenced this pull request Aug 21, 2026

Add test case for preserves tab indentation and trailing-newline

3906e81

#1226 (comment)

mu-hun requested a review from 105th August 21, 2026 13:17

105th approved these changes Aug 21, 2026

View reviewed changes

mu-hun added a commit that referenced this pull request Aug 24, 2026

Verify the parsed value is a non-null, non-array before accessing to …

bc62d1c

…`filter` prop #1226 (comment)

mu-hun added a commit that referenced this pull request Aug 24, 2026

Fix type guard to check is not array

1807f01

#1226 (comment)

mu-hun added a commit that referenced this pull request Aug 24, 2026

Preserve the original trailing newline if exist

3efe80f

#1226 (comment)

mu-hun added a commit that referenced this pull request Aug 24, 2026

Add test case for preserves tab indentation and trailing-newline

5564008

#1226 (comment)

mu-hun force-pushed the feature/#1208 branch from ba23ee8 to 9ca3f49 Compare August 24, 2026 07:42

zloyden approved these changes Aug 24, 2026

View reviewed changes

slvvko reviewed Aug 25, 2026

View reviewed changes

Comment thread scripts/build/strip-generated-meta.ts

mu-hun added a commit that referenced this pull request Sep 4, 2026

refactor: type METADATA_FILE_NAMES as readonly string[]

17427e4

The only module-level list without immutability typing. `readonly string[]` (not `as const`, which would break `.includes(entry.name)`). #1226 (comment)

mu-hun requested a review from maximtop September 4, 2026 06:20

maximtop reviewed Sep 5, 2026

View reviewed changes

slvvko approved these changes Sep 9, 2026

View reviewed changes

maximtop approved these changes Sep 9, 2026

View reviewed changes

Base automatically changed from feature/#1211 to master September 23, 2026 02:04

mu-hun added 6 commits September 23, 2026 14:46

feat: strip version/timeUpdated metadata from filters.json/filters.js

0fea2f6

Squashed from PR #1226 review-fixup commits: b59c6c4 1e32924 89b3595 e56cb35 36802bf defaee8 30913ec ecc7ce8 564ecf6 6acf8e8 6a22bb0 c85a00f 9ef7734 17427e4

test: cover strip-generated-meta edge cases

2ec06f1

Squashed from PR #1226 review-fixup commits: e4d91aa 471fc84 922c346 c2b206f 2dbc2d2 8a63c55

docs(AGENTS): codify trusted-vs-external validation policy, drop redu…

66085f1

…ndant type guard Squashed from PR #1226 review-fixup commits: 33bf686 e65cd82

fix(scripts): harden compare-build-output-against-master.sh verdict r…

b870594

…eporting Squashed from PR #1226 review-fixup commits: fbefade 67e6042 fa9e30a ea40dfd 598315a

mu-hun force-pushed the feature/#1208 branch from a6fba14 to 8ab7ce1 Compare September 23, 2026 05:55

mu-hun requested review from 105th and maximtop September 23, 2026 05:56

mu-hun requested a review from Alex-302 September 23, 2026 07:30

105th approved these changes Sep 23, 2026

View reviewed changes

maximtop approved these changes Sep 23, 2026

View reviewed changes

slvvko approved these changes Sep 24, 2026

View reviewed changes

Conversation

mu-hun commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mu-hun Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

105th left a comment

Choose a reason for hiding this comment

AI Code Review (open-code-review)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

105th Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

mu-hun Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

105th left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

105th left a comment

Choose a reason for hiding this comment

Code Review Summary

What this PR does

Verification highlights

Uh oh!

mu-hun commented Aug 24, 2026

Uh oh!

mu-hun commented Aug 24, 2026

Uh oh!

Alex-302 commented Aug 25, 2026

Uh oh!

mu-hun commented Aug 25, 2026

Uh oh!

Uh oh!

maximtop left a comment

Choose a reason for hiding this comment

Uh oh!

mu-hun commented Sep 7, 2026

Uh oh!

maximtop commented Sep 7, 2026

Uh oh!

mu-hun commented Sep 7, 2026

Uh oh!

maximtop left a comment

Choose a reason for hiding this comment

Uh oh!

Alex-302 commented Sep 9, 2026

Uh oh!

mu-hun commented Sep 11, 2026

Uh oh!

mu-hun commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mu-hun commented Sep 23, 2026

Uh oh!

105th left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AI Code Review (`open-code-review`)

mu-hun commented Sep 23, 2026 •

edited

Loading