feat(git): [#2175] accept declared symbolic links in the maintainer merge tool - #2184
Conversation
The merge tool refuses every symbolic link a merge introduces, and offers no way to admit one a repository carries on purpose. Describe the declaration file that admits them, .symlinks.json in the repository root, together with the rules the tool applies to it: which commits are checked, that the declaration is read from the final merged tree alone, that matching runs from the trees to the file and is byte-exact, that absolute and escaping targets are never accepted whatever the file says, and that the mechanism only runs when an invocation names a declaration path. Documenting the format before the tool implements it keeps the format reviewable on its own and gives the implementation a written contract to satisfy.
The merge tool refused every symbolic link in the merged tip, so a repository that carries a link on purpose could not reach signing at all and had no way to say which link it accepts. Add an optional --symlinks argument naming a declaration inside the merged tree, and check the links against it. The declaration is read from the local merge commit the tool has just created, never from the working directory or from a branch, so the links and the statement that admits them come from one reviewed tree and no state in the maintainer's environment can change the verdict. The check now walks every commit the merge introduces rather than the tip alone, because a link that appears mid-range and disappears before the tip still resolves on every checkout of the commit that carries it; each refusal names that commit. The final declaration judges the whole range, so no commit is admitted by a statement the merge result has withdrawn. Matching runs from the trees to the declaration and is byte-exact, an absolute or escaping target is refused whatever the declaration says, and every admitted link is printed with its reason before the maintainer signs. The argument carries no default: a run without it reads no declaration and behaves as the tool did before, so an exemption is always visible in the invocation that grants it.
…apper The merge tool holds no default declaration path, so a run that does not name one refuses every symbolic link the merge introduces. Pass the tracker's path from the wrapper, which is where repository-specific wiring already lives, so the exemption is stated once and is visible in the invocation that grants it. The value names a location in the merged tree the tool has yet to create, not a file in the maintainer's working directory, so the wrapper deliberately performs no existence check: it cannot observe that tree, and an absent declaration is not an error in the tool either.
The wrapper suite asserted the exact argument list the wrapper delegates, so naming the declaration path broke that assertion; update it and add a case showing the path is passed with no file of that name in the working tree, which is the tracker's own situation and the reason the wrapper cannot check for one. The tool's own behavior needs a repository to observe, so add a suite that builds one per case, together with the bare upstream that publishes the pull request to it. A URL rewrite sends the tool's fetch to that upstream, so the real fetch and merge paths run with no network and no GitHub API, and a driver replaces the three API calls with fixture data and answers the signing prompt with a refusal, which makes a run that passes the check exit 1 and a run the check refuses exit 4. The cases cover what the declaration decides: a declared link is admitted, an undeclared one, a different target, an escaping target and an absolute target each refuse; a declaration exempts nothing when it is only in the working directory, only on the target branch, unreadable, or when the invocation names none at all; a link that only an intermediate commit carries refuses and names that commit; and a removal keeps or drops the entry with the two outcomes the rules describe.
… policy The vendoring record asked for byte-identity with the origin copy and a new provenance hash after every local change, and the known-issues section justified leaving real defects unfixed by that requirement. The copy is not tracking its origin: its consumers are Torrust repositories whose needs Bitcoin Core does not share, and the first of those needs, a declaration for the symbolic links a repository accepts, cannot be met from the wrapper at all because the refusal lives inside the tool. Record the provenance the way a reader uses it: one permanent statement of where the file came from, under what license, and what it looked like on arrival. Everything after arrival is what git history is for, recorded more precisely than a hash in a README. Re-check the known upstream issues on that footing: they stay recorded because each is a separate defect with its own review, not because a policy forbids fixing them, and their line references are corrected for the file as it now stands. Also name the two test suites the tool now has in the coverage boundary.
…ners The merge workflow now admits declared symbolic links, and a maintainer meets that behavior mid-merge unless the skill describes it first. State what the merge output shows, that an undeclared link stops the run before there is anything to sign, and that the commit named in the message may be an intermediate one rather than the tip. The two properties that surprise a reader are worth naming explicitly: the declaration is read from the merged result rather than from the working copy, so a local file grants nothing, and removing a declared link takes two changes, so a stale-entry report on a removal pull request is the expected outcome rather than a defect.
…tion Add the issue-local verification record with the automatic checks, the manual scenarios M1-M8, and the acceptance evidence, and bring the specification up to date with what was implemented: task statuses, workflow checkpoints, scenario and acceptance tables, and progress-log entries for the readings the specification left open and how each was settled. Correct the stale-entry rule while doing so. Defining a stale entry as one matching no symbolic link in any checked commit contradicts AC7 and the removal rule that follows it, because the entry a removal pull request retains does match the link in its own pre-deletion commit and must still be reported as stale. An entry is stale when its path is not a symbolic link in the merged result, which is the reading both statements share and the one the tool implements.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds an opt-in, auditable mechanism for allowing specific symbolic links during maintainer merges by introducing a symlink declaration file read from the merged result and validating symlinks across the entire commit range introduced by a PR.
Changes:
- Add
--symlinks <tree path>togithub-merge.py, read/parse a declaration from the merge commit, and check symlinks acrosspull/<n>/base..pull/<n>/headplus the merge commit. - Update
merge-pull-request.shto always delegate--symlinks .symlinks.json(tree path, no working-tree existence check). - Add comprehensive tests and documentation/spec updates, including manual verification evidence.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| contrib/dev-tools/git/github-merge.py | Implements declaration parsing and commit-range symlink enforcement behind --symlinks. |
| contrib/dev-tools/git/merge-pull-request.sh | Delegates the declaration tree path to the vendored tool unconditionally. |
| contrib/dev-tools/git/tests/test-github-merge-symlinks.py | Adds deterministic integration-style tests for the tool’s symlink behavior. |
| contrib/dev-tools/git/tests/test-merge-pull-request.sh | Updates wrapper delegation assertions and adds a no-working-tree-file delegation case. |
| contrib/dev-tools/git/README-github-merge.md | Documents the declaration format, rule set, and coverage boundary updates. |
| .github/skills/dev/git-workflow/merge-pull-request/SKILL.md | Updates maintainer guidance for symlink behavior and tests. |
| docs/issues/open/2175-merge-tool-symlink-exceptions/ISSUE.md | Marks tasks/AC as done and updates the spec/progress log. |
| docs/issues/open/2175-merge-tool-symlink-exceptions/verification.md | Adds recorded evidence for automated and manual verification scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2184 +/- ##
===========================================
+ Coverage 84.66% 84.97% +0.31%
===========================================
Files 351 351
Lines 29657 30211 +554
Branches 29657 30211 +554
===========================================
+ Hits 25108 25671 +563
+ Misses 4197 4168 -29
- Partials 352 372 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
Blocking: process the Copilot review threads
The three Copilot suggestions must be addressed before this PR can merge. Please process every Copilot review thread strictly according to the repository process-copilot-suggestions workflow: assess each suggestion against the code and applicable requirements, make the minimal correction where warranted (or record a substantiated no-action decision), reply to each thread with the rationale, resolve it individually, and update the required tracker record.
The open findings concern:
- per-symlink subprocess overhead during commit-range validation;
errors="replace"undermining the specified byte-exact target comparison; and- unescaped path or target values permitting misleading terminal diagnostics.
Please run the prescribed validation after addressing them and report the result.
|
The location and name of '.symlinks.json' is a repo convention. The tool itself has no notion of the actual location and name of the symlinks manifest. - That the file is accepted is defined by the namespace the tool encodes. Will correct issue and pull request to make this fact explicit. |
A path, a link target, and a declaration reason are all chosen by whoever wrote the commit, and a value may legally carry a newline or a terminal escape. Interpolated into the report as it stands, such a value forges lines the tool never wrote: a target ending in a newline and the text of a refusal prints an error for a file that was never refused, and the maintainer reads it moments before signing. Paths and targets are now quoted and escaped for display, and the reason passes through the sanitizer the tool already applies to text it did not write. An ordinary path or target renders exactly as before, so the documented message shapes are unchanged; the escaping is a rendering only and never reaches the comparison that decides which links are admitted.
The rules say a declared target must equal the link's content byte for byte, and the check decided the match on a decoded rendering instead. The link blob was decoded with a replacing decoder, so every byte that is not valid UTF-8 became the same replacement character: distinct targets collapsed onto one string, and a declaration naming that string admitted a link whose content it does not describe. The path was decoded from a listing git renders rather than from the tree, which quotes control characters always and bytes outside ASCII under the default configuration, so a declaration naming a path outside ASCII could not match the link it describes. Paths and targets are now read and compared as the bytes the tree carries, and the declaration's strings are encoded once to the bytes they stand for. A target that is not valid UTF-8 has no declaration it can equal and is refused, two links whose contents differ can no longer match one entry, and a path outside ASCII matches the entry that names it. Listing with '-z' is what makes the path available at all rather than its rendering.
The range walk read a link's content from the object store once per commit that carried it, so the cost of the check grew with commits times links even though a link usually keeps the same content across a whole pull request. Each read is its own process, and process startup dominates a read this small. Identical content is one blob whatever the path or the commit, so the walk now carries the contents it has already read and asks the object store only for the ones it has not. Keyed by object id, the reuse cannot answer for a link whose content changed: a changed target is a different blob and is read, which is what the existing case for a target changing inside the range asserts. Measured over a fifty-commit range: with one link per commit, 262 ms to 143 ms; with twenty, 2778 ms to 188 ms. What is left is one tree listing per commit, which the check cannot avoid, because whether a commit carries a link is a property of its own tree.
…nvention The tool holds no default declaration path, and the documents said so where they discussed the argument while still describing the file as a root file at a fixed name where a reader first meets it. That reads as though `.symlinks.json` at the repository root were part of the mechanism, and it is not: the tool reads whatever tree path `--symlinks` names and attaches no meaning to the name, while this repository's choice of path is stated once, in the wrapper's argument. The objective, the declaration-format section, and the merge skill now say that where they describe the file, so a repository adopting the workflow reads the same fact wherever it starts.
Records the round: the three review threads, what each was decided to be and why, the commit that carries each fix, the measurements behind accepting one suggestion in part and declining the batch reader it proposed, and the reply that states each decision on the thread itself.
|
The three Copilot threads are processed per Decisions, in the order they were handled:
A fourth commit, a0f69c7, makes explicit in the README, the merge skill, and the specification that the declaration's name and location are a repository convention stated once in Validation at 82cbdb4 on the build host: |
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
Blocking: validate the declaration format contract
contrib/dev-tools/git/github-merge.py currently accepts an arbitrary JSON object containing a symlinks array. It does not validate the required manifest namespace (com.torrust.repository.symlinks) or its supported version before constructing exemptions.
This contradicts the documented format in contrib/dev-tools/git/README-github-merge.md and the accepted contract in issue #2175. It means a foreign or malformed manifest could authorize symbolic links.
Please validate the exact namespace and supported version before reading declarations, and fail closed (admit no exemptions) whenever either field is missing or unsupported. Add focused negative tests covering missing, incorrect, and unsupported namespace/version values, then run the relevant test suites and prescribed validation gate.
The declaration's header states which format a document belongs to and which revision of that format it was written to, and the check read neither. Any JSON object carrying a 'symlinks' array was read as a declaration of this format, so a document written to some other format, or to a revision whose rules this tool has not been taught, could authorize a symbolic link on the strength of a field name it happens to share. Both header fields are now checked before a single entry is read: 'namespace' must be exactly 'com.torrust.repository.symlinks' and 'version' exactly [1, 0, 0]. A document that omits either field, or carries another value for it, exempts nothing and takes the path a declaration that cannot be read already takes, so the report names the field that disagreed and every link refuses exactly as it does with no declaration at all. The comparison is exact and type-strict. Python compares True to 1 and 1 to 1.0 as equal, while JSON's true, 1 and 1.0 are three different documents, so a comparison resting on equality alone would read [true, 0, 0] as the supported version. An unsupported version fails closed rather than being read under the rules of the version this tool does know, which leaves a later revision of the format to teach the tool its version rather than pass unread. The values a message reports come out of a tree, so each is rendered as the JSON it came from, which escapes every control character and cannot forge a line of the report.
Seven cases fix the header's meaning: a declaration missing its namespace, one naming another format, one whose namespace is that string inside an array, a declaration missing its version, one naming an unsupported version, one whose version is a string rather than an integer triple, and one whose version carries a boolean where an integer is documented. That last case is the one a comparison resting on Python equality alone would admit. Each commits a symbolic link together with an entry that would admit it, so the header is the only thing between the link and an exemption, and each asserts both halves of the behaviour: the link is refused with the error naming the commit that carries it, and the report says which field disagreed. Asserting the warning alone would pass against a tool that warned and exempted anyway. The declaration builder takes the header fields as arguments defaulting to the documented values, with a sentinel for a field the document does not carry, so a case about the header builds exactly the document it is about and the accepting case keeps building the documented one.
The format section showed the namespace and the version without saying they are read, and the rules covered only a declaration that cannot be parsed, so a reader could not tell whether a document of another format was one of those. Both documents now carry the rule where a reader meets the fields: the header is checked before any entry, the values are matched as the JSON values they are, and a document that omits either field or carries another value exempts nothing. The specification's progress log records the reversal of the earlier reading, which held the namespace documented but not enforced because no rule made the tool validate it, together with why an unsupported version has to fail closed.
|
Thank you, you are right: the tool accepted any JSON object carrying a What is validated. How it fails closed. A missing field, a wrong value, and a wrong-typed value all return the same The message names the field, the value found, and the value expected. Values out of a tree are chosen by whoever wrote the commit, so each is rendered as the JSON it came from, which escapes every control character and everything outside ASCII and cannot forge a line of the report. Two readings the documents left open, recorded in the specification's progress log:
Commits
Tests. Seven focused cases in
The accepting case is unchanged: Documentation. The vendoring README's rules now state that the header is checked before any entry and that a mismatch exempts nothing, and the format section says both fields are required and read exactly. The issue #2175 specification carries the matching rule, and its progress log records the reversal of the earlier reading that held the namespace documented but not enforced. Validation, all on the branch tip:
|
|
Thanks — the namespace/version validation and associated tests address the prior finding. One remaining blocker from the focused re-review: Python Please reject these constants explicitly (for example, via a rejecting |
A declaration is read with Python's JSON reader, which accepts NaN, Infinity and -Infinity although JSON defines none of them. A document carrying one of those was therefore read here and would be refused by every conforming reader elsewhere, so a file that is not valid JSON could authorize a symbolic link, and the rule that a declaration which cannot be read exempts nothing never applied to it. The reader is now told to refuse all three. The refusal is a ValueError, which is what invalid JSON already raises at this call, so the document takes the one path a broken declaration already takes: nothing is declared, every link refuses exactly as it does with no declaration at all, and the report names the constant that was found rather than a second failure surface. Position in the document is not a distinction the reader can make, so a constant in the header, in a top-level field, or inside an entry all fail the same way and are caught before a single entry is read. The two remaining reads of JSON in this tool parse GitHub API responses rather than repository content. They are a different trust class: the response is not the input that removes a refusal, it is discarded on any exception already, and no value it carries can exempt a link, so they are left as they are.
Ten cases fix what a declaration may spell. Nine commit a symbolic link together with an entry that would admit it, under a document whose only fault is NaN, Infinity or -Infinity, each of the three constants placed in the version triple, in a top-level field, and inside an entry. The tenth is the same document with no constant in it, admitting that same link, so each of the nine refusals is the constant rather than the document those cases vary. The two positions outside the header are the ones that decide the behaviour: nothing else in those documents disagrees, so a reader that admitted the constant would exempt the link. The version triple covers the third position and shows the refusal comes before any rule of this format is applied, since the report names the constant rather than the field the header check would otherwise have refused. Each case asserts both halves: the link is refused with the error naming the commit that carries it, and the report carries the whole warning line naming the constant that was found. Asserting the warning alone would pass against a tool that warned and exempted anyway. The documents are written as text rather than serialized from Python values, because a constant JSON does not define is a violation of the grammar rather than a value a serializer would be asked for, and what a case is about is exactly the bytes its commit carries.
The rules said a declaration that cannot be read exempts nothing, and left the reader's own latitude unstated, so a reader could not tell that a document spelling NaN, Infinity or -Infinity was one of those rather than a document those rules had nothing to say about. Both documents now carry the rule beside the header rule it precedes: the document is read as JSON and nothing wider, the three constants JSON does not define are refused wherever they appear, and such a document exempts nothing and reports the constant it carried. The specification's progress log records why the gap existed, that the reader the tool is built on accepts all three by default, and why the tool's two remaining reads of JSON, which parse GitHub API responses, are a different trust class and are unchanged.
|
Thanks — fixed at 4bff469. The declaration parse now passes a and every link then refuses with the usual Ten new tests in
Each of the nine commits the link together with an entry that would admit it, so the constant is the only thing between the link and an exemption, and each asserts both the refusal naming the carrying commit and the full warning line — a tool that warned and exempted anyway would fail. The two positions outside the header are the ones that decide the behaviour: nothing else in those documents disagrees, so a reader admitting the constant would exempt the link. The tenth is the same document without a constant, still accepted, which fixes that the nine refusals are the constant rather than the template. The tool's two other JSON reads are in Gates at |
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
Final re-review complete. The declaration parser now rejects non-standard JSON constants fail-closed, the focused coverage verifies those paths, and the implementation and documentation align with issue #2175. Approved.
|
ACK 4bff469 |
Closes #2175
Summary
The vendored maintainer merge tool refuses a merge whose tree contains any symbolic link, with no way to admit a link a repository carries on purpose. That refusal currently blocks every merge in
torrust/torrust-index, whose tree carries one deliberate link (.dockerignorepointing at.containerignore, so Docker and Podman share a single ignore list), and it will block any Torrust repository that ever declares one.This adds an explicit, auditable exception: a repository declares the links it accepts in a JSON file it commits, the tool reads that declaration out of the merged tree it has just produced at the tree path the invocation names, and it exempts exactly the declared links whose target matches byte for byte. Everything else refuses exactly as before. The check also widens from the merged tip to every commit the merge introduces, judged against that one final declaration.
The mechanism is opt-in per invocation, and the declaration's name and location are the caller's, not the tool's. The tool holds no default path and knows no file name: it reads the tree path
--symlinksgives it, so a run without the argument reads nothing and behaves exactly as the tool did before..symlinks.jsonat the repository root is this repository's convention, stated in exactly one place, the unconditional--symlinks .symlinks.jsoninmerge-pull-request.sh; a repository adopting the workflow may keep its declaration anywhere in its tree and name it in its own wrapper. This repository's tree carries no symbolic link and this PR adds no declaration file, so no tracker merge changes behavior.What changed, per file
contrib/dev-tools/git/github-merge.py— new--symlinks <tree path>argument with no default value, validated as a repository-relative tree path (an absolute value or one containing..is a usage error, exit2). The tip-only refusal is replaced by a check over every commit inpull/<n>/base..pull/<n>/headplus the local merge commit, each listed withgit ls-tree --full-tree -r -z <commit>under the same mode mask as before, so the paths compared are the tree's own bytes rather than git's rendering of them, and every path and target comparison runs on bytes. The declaration is read once, from the merge commit, withgit show <merge commit>:<path>. New helpers:get_symlink_entries,read_symlink_declaration,symlink_declaration_path_error,symlink_target_is_confined,check_symlinks;get_symlink_filesis replaced by the first of these.contrib/dev-tools/git/merge-pull-request.sh— passes--symlinks .symlinks.jsonbefore the positional arguments of its finalexec python3, unconditionally and with no filesystem check.contrib/dev-tools/git/README-github-merge.md— new "Declared Symbolic Links" section with the declaration format and the full rule set; the provenance section rewritten to the single permanent statement; the known-upstream-issues section re-checked and its line references corrected; the coverage boundary updated for the new suite.contrib/dev-tools/git/tests/test-github-merge-symlinks.py— new suite, 23 tests, covering the tool's own behaviour, including targets and paths outside ASCII, a target that is not valid UTF-8, and control characters in reported values.contrib/dev-tools/git/tests/test-merge-pull-request.sh— the delegation assertion updated for the new argument list, plus a case asserting the path is delegated with no such file in the working tree..github/skills/dev/git-workflow/merge-pull-request/SKILL.md— a "Symbolic Links in the Merge" section for maintainers.docs/issues/open/2175-merge-tool-symlink-exceptions/— the specification brought up to date, and a newverification.mdholding the evidence.How the architectural decisions are honoured
AD1 — the tool is maintained here, not tracked against upstream. The refusal lives inside
github-merge.py, before the tree hash is taken, and no wrapper argument reaches it, so the feature is implemented by editing the tool directly. The README now carries exactly one provenance statement: the file came from the Bitcoin Core developers'github-merge.py(MIT, retained ingithub-merge-COPYING), with SHA-256e390eb014131f3183a2cba642134974a6b09b19a65322d17dd7c81cf4ffbaad2, in commit833a4160e5753d54cde47bcc4bed25df7a04c0f6(2026-07-23). The byte-identity requirement, the per-change re-hashing, and the local-change policy are gone; git history records what changed after arrival more precisely than a hash could. The known upstream defects stay recorded rather than fixed, now because each is a separate defect with its own review rather than because a policy forbids touching the file.AD2 — no default path inside the tool.
--symlinkshasdefault=None, so an omitted argument is distinguishable from any path and disables declaration processing entirely: no file is read, nothing is exempted, and no accepted-link or stale-entry output appears, even when the merged tree carries a.symlinks.jsonthat would have declared the link. The wrapper states the repository's path once. Scenario M8 inverification.mddemonstrates exactly this.Validation
All runs on the project build server at idle scheduling priority; the first four re-run at the review-round tip, the pre-push gate from the original tip 80562d1.
python3 contrib/dev-tools/git/tests/test-github-merge-symlinks.py(23 tests)bash contrib/dev-tools/git/tests/test-merge-pull-request.shlinter allTORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh(6 of 6 steps)TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-push.sh(4 of 4 steps)Both suites leave the working tree clean, which the merge workflow depends on since it refuses to start on a dirty tree.
The eight manual scenarios of the specification were executed against fixture repositories and their output is recorded in
docs/issues/open/2175-merge-tool-symlink-exceptions/verification.md: a declared link is accepted and printed with its reason before the signing prompt (M1); an undeclared link refuses with exit4, naming each carrying commit (M2); a declared absolute target and a declared..target both refuse (M3); a missing declaration file, reached through the wrapper, reproduces today's refusal with no error of its own (M4); a declaration present only in the working directory or only on the base branch exempts nothing (M5); a link carried only by an intermediate commit refuses and names that commit, which a tip-only check would have missed (M6); a removal pull request refuses when it drops the entry and passes with a stale-entry report when it retains it (M7); and an omitted argument refuses a declared link and prints no accepted or stale output (M8).Notes for review
--symlinksvalue is an ordinary usage error, and thenamespacefield is documented but not enforced.torrust/torrust-indexadds its own.symlinks.jsonand its own wrapper argument once this lands; mirroring is deliberately out of scope here.