Conversation
|
🤖 Here's your preview: https://k7oic-piaaa-aaaam-ai7uq-cai.icp0.io |
1d0da3e to
9f1e3e3
Compare
There was a problem hiding this comment.
🔵 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
TAGan ancestor ofPIN, so lines 74-77 exit early withneeded=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.jsskips the synced static-site directory incheckEmdashandcheckForbiddenPatterns(scripts/validate.js:49-50and69-71), so it does not reject these characters ordfxthere. The sync can still refuse them, but the diagnostic should attribute the requirement to the repository's sync/content contract rather than claimingvalidate.jsenforces 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 sayshyphenated "tamper-proof", so the diagnostic misidentifies the upstream deviation. Use wording that covers both spellings, such asnoncanonical "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.
9641696 to
818d968
Compare
|
Copilot's three suppressed comments, checked against the code. One was right, two were not. Right: the house-style label. Wrong: Wrong: a commit pin is stranded forever. The claim is that the next release containing the synced commit makes |
There was a problem hiding this comment.
🔵 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>. Thereforerev-parse "$INPUT_REF^{commit}"fails for a valid non-default branch such asdocs/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$TAGwas 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
818d968 to
2d4f2c5
Compare
|
Both of Copilot's suppressed comments were right this round, and both are fixed in Annotated tags. The tags here are annotated ( Branch refs. Correct: the clone creates Exercised against a real clone of the upstream repo, all six input forms:
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. |
There was a problem hiding this comment.
🟡 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..TAGis empty, but the workflow enters the manual-ref branch and creates a pin-only PR that movesupstream.jsonbackwards to the older commit, causing the next weekly run to reprocess later releases. Treat any ref already contained byPIN_COMMITas 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
2d4f2c5 to
152b6fc
Compare
There was a problem hiding this comment.
🟢 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
dfxupstream. 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 mentiondfx).
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.jsdoes not rejectdfx, 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 tovalidate.jsandAGENTS.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
Summary
Stacked on #400. Three changes to the static-site sync.
dfxtranslation and the brand normalizations are gone. Link mapping, thesource_repo/source_reffrontmatter and the do-not-edit marker stay.dfx), naming the page, for upstream to fix. Softer house style is reported only.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..agents/upstream-tracking.mdupdated to match.Verified at
59641fc: output byte-identical to the eight upstream files apart from the marker, two provenance fields and three link rewrites;validateandbuildpass.The sync itself is not included. Merge this, then dispatch with
ref=59641fc.