Skip to content

feat: sync static-site docs verbatim, and allow syncing a ref by hand - #401

Open
marc0olo wants to merge 5 commits into
infra/anchor-slug-code-spansfrom
infra/static-site-sync-network-flag
Open

marc0olo wants to merge 5 commits into
infra/anchor-slug-code-spansfrom
infra/static-site-sync-network-flag

Conversation

@marc0olo

@marc0olo marc0olo commented Sep 18, 2026

Copy link
Copy Markdown
Member

Summary

Stacked on #400. Three changes to the static-site sync.

  • Pages land verbatim. The dfx translation and the brand normalizations are gone. Link mapping, the source_repo / source_ref frontmatter and the do-not-edit marker stay.
  • Banned content stops the sync (em dash, en-dash separator, dfx), naming the page, for upstream to fix. Softer house style is reported only.
  • The workflow takes an optional ref (sha, tag or branch) to sync a docs fix that upstream has shipped but not released. Dispatching the pinned release is a no-op; a ref behind the pin runs but warns that the pin moves backwards.
  • A release that changes no page opens no PR, unless the pin is a commit, where moving it onto a tag is the point.
  • .agents/upstream-tracking.md updated to match.

Verified at 59641fc: output byte-identical to the eight upstream files apart from the marker, two provenance fields and three link rewrites; validate and build pass.

The sync itself is not included. Merge this, then dispatch with ref=59641fc.

@marc0olo
marc0olo requested a review from a team as a code owner September 18, 2026 12:05
@marc0olo
marc0olo added this pull request to stack #402 September 18, 2026 12:05
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown

🤖 Here's your preview: https://k7oic-piaaa-aaaam-ai7uq-cai.icp0.io

@marc0olo
marc0olo marked this pull request as draft September 18, 2026 12:11
@marc0olo
marc0olo force-pushed the infra/static-site-sync-network-flag branch from 1d0da3e to 9f1e3e3 Compare September 18, 2026 12:39
@marc0olo marc0olo changed the title fix: target the network, not an environment, in synced icp commands feat: sync static-site docs verbatim, and allow syncing a ref by hand Sep 18, 2026
@marc0olo
marc0olo requested a lite review from Copilot September 18, 2026 13:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The workflow can leave a manual commit pin permanently stale instead of moving it to a containing release.

Pull request overview

Updates static-site synchronization to preserve upstream prose, support manual refs, and skip no-op release syncs.

Changes:

  • Adds banned-content checks and style reporting.
  • Adds manual workflow ref handling.
  • Updates pinning and no-op PR behavior.
File summaries
File Review summary
scripts/sync-static-site.mjs Sync behavior and diagnostics updated; minor wording and attribution corrections remain.
.sources/upstream.json Updates the pinning policy.
.github/workflows/sync-static-site.yml Adds manual ref and no-op decision logic; commit-pin release handling needs correction.
Review details

Suppressed comments (3)

.github/workflows/sync-static-site.yml:63

  • A manual commit pin is never moved back onto a release by the weekly path promised in the PR description. After a docs-only commit is synced, the next release containing it makes TAG an ancestor of PIN, so lines 74-77 exit early with needed=false; the pin remains the commit forever and the later tag is never recorded. The release check needs to distinguish a commit pin and select the containing release (or otherwise explicitly create the pin-only PR) before applying this early exit.
            echo "Manual ref: $INPUT_REF resolved to $TAG. Pinned: $PIN."
            if [ "$(git -C /tmp/certified-assets rev-parse "$TAG")" = \
                 "$(git -C /tmp/certified-assets rev-parse "$PIN")" ]; then
              echo "That is the pin already. Nothing to sync."
              echo "needed=false" >> $GITHUB_OUTPUT
              exit 0
            fi

scripts/sync-static-site.mjs:345

  • This explanation is not accurate for the current validator: scripts/validate.js skips the synced static-site directory in checkEmdash and checkForbiddenPatterns (scripts/validate.js:49-50 and 69-71), so it does not reject these characters or dfx there. The sync can still refuse them, but the diagnostic should attribute the requirement to the repository's sync/content contract rather than claiming validate.js enforces it on every page.
        `${file}: holds an em dash or an en-dash separator, which ` +
          `scripts/validate.js rejects on every page in this repo. Fix it ` +
          `upstream; this sync does not rewrite prose.`

scripts/sync-static-site.mjs:94

  • When the match is tamper proof (with a space), this report still says hyphenated "tamper-proof", so the diagnostic misidentifies the upstream deviation. Use wording that covers both spellings, such as noncanonical "tamperproof", so maintainers know exactly what to fix upstream.
const HOUSE_STYLE = [{ re: /tamper[- ]proof/i, what: 'hyphenated "tamper-proof"' }];
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

@marc0olo
marc0olo force-pushed the infra/static-site-sync-network-flag branch from 9641696 to 818d968 Compare September 18, 2026 13:25
@marc0olo

Copy link
Copy Markdown
Member Author

Copilot's three suppressed comments, checked against the code. One was right, two were not.

Right: the house-style label. /tamper[- ]proof/i matches a space too, so hyphenated "tamper-proof" misnamed half the cases. It now reports "tamperproof" split in two, which covers both. Fixed.

Wrong: validate.js skips the synced tree. It cited isSynced at lines 49-50 and 69-71, but SYNCED holds only docs/languages/motoko, so checkEmdash and checkForbiddenPatterns do run over docs/guides/frontends/static-site/. I put an em dash and a dfx command into a synced page and ran the validator, which reported them. So the diagnostic's attribution stands, and this is also what makes the link rewriting non-optional: that same check rejects docs.internetcomputer.org, which is how upstream writes these links.

Wrong: a commit pin is stranded forever. The claim is that the next release containing the synced commit makes TAG an ancestor of PIN, hitting the early exit. That has the direction backwards: if the release contains the commit, the commit is the ancestor, not the tag, so merge-base --is-ancestor "$TAG" "$PIN" is false and the run proceeds. I checked it on a real clone with PIN=8c7cfc2 (a commit before the v0.3.2 tag) and TAG=v0.3.2, which contains it: the check returns false, the run continues, and the pin-only branch opens the PR that moves the pin onto the tag. The early exit fires only when the latest tag is genuinely behind the pin, which is the case it exists for: syncing it would publish older prose.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Resolve non-default branch refs and annotated tag commit comparisons in the workflow.

Review details

Suppressed comments (2)

.github/workflows/sync-static-site.yml:53

  • The workflow description promises that a branch can be supplied, but a normal clone only creates local refs for the default branch; other branches are available as origin/<branch>. Therefore rev-parse "$INPUT_REF^{commit}" fails for a valid non-default branch such as docs/fix, so the manual-sync path cannot handle one of its documented input forms. Resolve the local ref first and fall back to the corresponding remote-tracking ref (or fetch/resolve it explicitly).
            TAG=$(git -C /tmp/certified-assets rev-parse --short "$INPUT_REF^{commit}") || {

.github/workflows/sync-static-site.yml:59

  • When the configured pin is an annotated release tag, git rev-parse "$PIN" returns the tag object's ID, while $TAG was resolved with ^{commit} and is the commit ID. Therefore dispatching with that tag (or its commit) is not recognized as the documented no-op and can open an unnecessary pin-only PR. Resolve the pinned side as a commit as well.
            if [ "$(git -C /tmp/certified-assets rev-parse "$TAG")" = \
                 "$(git -C /tmp/certified-assets rev-parse "$PIN")" ]; then
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@marc0olo
marc0olo force-pushed the infra/static-site-sync-network-flag branch from 818d968 to 2d4f2c5 Compare September 18, 2026 13:48
@marc0olo

Copy link
Copy Markdown
Member Author

Both of Copilot's suppressed comments were right this round, and both are fixed in 2d4f2c5. They are the same kind of bug: a ref was compared or resolved without being peeled to a commit.

Annotated tags. The tags here are annotated (git cat-file -t v0.3.3 is tag), so rev-parse v0.3.3 returns the tag object 985594d while $TAG held the commit ac2a8e7. The equality check could never fire. The consequence was worse than a missed no-op: dispatching the pinned tag read as a change and would have rewritten the pin from v0.3.3 to its own commit sha, moving a tag pin onto a commit for no reason, which is the one thing the pin policy exists to avoid. Both sides are now peeled with ^{commit}.

Branch refs. Correct: the clone creates refs/heads/main and leaves every other branch reachable only as origin/<branch>, so a bare feat/sns-e2e failed with "Needed a single revision" while the input description advertised branches. It now tries the ref as given and then under origin/.

Exercised against a real clone of the upstream repo, all six input forms:

input pin result
59641fc 65c0f32 proceed → 59641fc
v0.3.3 (annotated tag) v0.3.3 no-op
ac2a8e7 (that tag's commit) v0.3.3 no-op
feat/sns-e2e (non-default branch) v0.3.3 proceed → 18f93c4
main v0.3.3 proceed → 59641fc
no-such-ref v0.3.3 error, run fails

The third row is the one worth naming: dispatching a tag's own commit while pinned at that tag is also a no-op now, so neither spelling of the pinned release can quietly turn a tag pin into a commit pin.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Ancestor refs can incorrectly create backward-moving pin PRs, and workflow documentation is stale.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

.github/workflows/sync-static-site.yml:78

  • A manually supplied ref that is an ancestor of the current pin is treated as a change unless it is exactly equal. In that case diff PIN..TAG is empty, but the workflow enters the manual-ref branch and creates a pin-only PR that moves upstream.json backwards to the older commit, causing the next weekly run to reprocess later releases. Treat any ref already contained by PIN_COMMIT as a no-op, not only an exact match.
            if [ "$REF_COMMIT" = "$PIN_COMMIT" ]; then
              echo "That is the pin already. Nothing to sync."
              echo "needed=false" >> $GITHUB_OUTPUT
              exit 0
            fi
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread .sources/upstream.json
@marc0olo
marc0olo force-pushed the infra/static-site-sync-network-flag branch from 2d4f2c5 to 152b6fc Compare September 18, 2026 14:07
@marc0olo
marc0olo requested a lite review from Copilot September 18, 2026 14:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

Only non-blocking documentation comments remain.

Review details

Suppressed comments (2)

.agents/upstream-tracking.md:278

  • The sync no longer normalizes banned characters: it now rejects em dashes, en-dash separators, and dfx upstream. This existing failure description still says a banned character "survived normalization", which sends maintainers looking for a transformation that no longer exists. Update it to describe the current fail-fast behavior (and mention dfx).
To sync a ref rather than a release, dispatch the workflow with `ref`: a sha,
tag, or branch. That is for a docs fix that has shipped upstream but is not in a
release, and it leaves the pin on a commit until the next release moves it onto
a tag. The release checks do not apply to a dispatched ref, so it can also move
the pin backwards, which a rollback wants and a mistyped sha does not: the run
says so and the PR body repeats it.

.github/workflows/sync-static-site.yml:42

  • This comment overstates the repository's enforcement: scripts/validate.js does not reject dfx, and it does not reject en-dash separators; the latter is also used in existing docs for numeric ranges. The workflow may intentionally enforce these additional synced-tree constraints, but please describe them as this sync's contract rather than attributing all three bans to validate.js and AGENTS.md, so maintainers do not mistake the source of truth.
        id: check
        env:
          GH_TOKEN: ${{ steps.app-token.outputs.token }}
          # A dispatch input reaches the shell as data, never as script.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@marc0olo
marc0olo marked this pull request as ready for review September 18, 2026 14:19
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.

2 participants