fix(review): retry a failed hosted replace as a create in explain-change

[kyle-sexton]

Requested by Kyle · project thread

Closes #6749

Summary

Before: if a hosted digest page was deleted outside the skill (for example with pages-publish --delete <id>), the sidecar still held its id. The next rebuild of the same pull request passed --id <id>, the host refused the replace, and the publish failed. The page then stayed a local file.

After: a failed replace is retried once without --id. The rebuild publishes a new page, the sidecar takes the new id, and the old id goes under stale and is deleted, the same as for any other changed id.

Fix

• plugins/review/skills/explain-change/scripts/publish-hosted.mjs: when the sidecar's id is passed and pages-publish exits 6, the script runs pages-publish once more without --id. The rest of the flow is unchanged (JSON check, sidecar write, stale delete, public-landing guard). Nothing is retried after pages-publish refuses credential-shaped content (exit 4), after pages-publish fails to start, or after a first publish that had no --id.
• plugins/review/tests/explain-change.test.mjs: the fake pages-publish takes a FAKE_REPLACE_EXIT that applies only to calls with --id. Two new tests:
  1. A failed replace followed by a successful create gives the new id in the sidecar and deletes the old id.
  2. A refusal, a create that also fails, and a failed first publish each get no retry, and the sidecar keeps the old id.
• docs/conventions/rendered-views/README.md (sidecar bullet) and its CHANGELOG.md record the retry.
• Changelog fragment: review, patch.

Defaults I picked where the issue was silent:

• Any exit 6 on a replace triggers the retry, not only a 404. The pages-publish contract (the "The pages-publish command" section of the rendered-views convention) gives a missing id no exit code of its own: a 404 is exit 6, like any other upload failure. Reading stderr would tie the lane to a message the contract does not define. After a retry that succeeds, the sidecar and stale handling leave exactly one live page whichever failure caused the retry. The cost is that a transient replace failure can move the page to a new URL.
• The fix relies on the contract's "--delete exits 0 when the page is gone". The old id is listed under stale, then deleted. If the host answers a delete of an already-deleted id with an error, that stale entry is retried on every publish and reported as "delete failed". The installed pages-publish treats any status other than 204 as exit 6, so this depends on what the host returns for a missing id. I could not check that tonight.

Verification

• node --test plugins/review/tests/explain-change.test.mjs: 105 tests, 104 passed, 0 failed, 1 skipped (already skipped before this change).
• The two new tests run against origin/main's publish-hosted.mjs (script stashed, tests kept): both fail. With the fix, both pass.
• scripts/affected-tests.sh --base origin/main --run: explain-change.test.sh passes; explain-change.test.mjs is node's lane and passed in the run above.
• scripts/check-changelog-fragments.sh --check and --check-required origin/main; scripts/check-changelog-parity.sh --check, --check-bump origin/main and --check-preserved origin/main: all pass.
• node_modules/.bin/markdownlint-cli2 on the two convention files and the fragment: 0 issues.
• scripts/sync-shared-copies.sh --check: all 411 copies match.
• Diff checked for secrets and machine paths: none.

Related

• fix(review): explain-change artifact medium should look up repo visibility itself #6951 and open PR fix(review): take the artifact gate's visibility only from the view facts #7087 (the explain-change visibility lookup) touch other files in this skill (digest-policy.mjs, SKILL.md). fix(review): explain-change artifact medium should look up repo visibility itself #6951 names this issue as related.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR

---

Generated by Claude Code

[kyle-sexton]

ci-status is red because this PR is a draft, not because of the change. On a draft every lint and test lane skips (lint-repo, test-bash, test-node, test-python, check-skills, check-plugins), and the aggregate counts a skip as not passing. See .github/workflows/pr-require-checks.yml:507-513.

The lanes run once the PR is marked ready. That flip waits for Kyle, because it also starts the review lanes. Until then, the local runs listed under Verification in the PR body are the signal.

---

Generated by Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T04:46:18.686912Z	773c4e5	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 1m 35s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Review publish-hosted.mjs retry logic for security issues
• Review test changes
• Review doc/changelog changes
• Post findings

No security issues found.

Scope reviewed: plugins/review/skills/explain-change/scripts/publish-hosted.mjs — the only production-code change in this PR. The retry adds one extra pagesPublish call (dropping --id) only when the first call used --id and exited 6. Checked:

• No new injection surface. old.id used in the retry's --id arg is read from the sidecar through readSidecar/valid(), which already requires it to match the ID regex (^[A-Za-z0-9_-]{22}$) before it's trusted; args are passed to spawnSync as an array (no shell), so no command injection either way.
• No re-vetting gap. The retry resends the same already-gated page content (publishGate/validateView ran once, before either call); the two calls differ only in the presence of --id, so no unvetted content reaches pages-publish on the second attempt.
• No destination/visibility change across the retry. Both calls use identical args (page, --visibility, gate.destination); the retry can't move content to a different visibility than what was already gated.
• Exit-4 (credential refusal) and non-6 failures are explicitly excluded from the retry path (replacing && !run.error && run.status === 6), matching the PR's stated intent — a refusal is never silently retried as a create.
• The known tradeoff the PR body already calls out (any exit 6 on a replace, not just a 404-shaped one, triggers the retry) is a reliability/correctness tradeoff, not a security gap: worst case is an extra hosted page, not a privilege or data exposure change.

Test and docs changes are test-harness/prose only and introduce no production behavior.

claude-lane-summary: lane=claude-security-review sha=773c4e52cb914403c9097fda1585832da66c3ed9 run=38347745775

[claude]

Claude finished @kyle-sexton's task in 1m 53s —— View job

---

Code review: no findings

• Gather context (PR diff, changed files)
• Review publish-hosted.mjs retry logic
• Review test coverage in explain-change.test.mjs
• Review docs/changelog updates
• Post findings

Reviewed the retry-as-create fix in
publish-hosted.mjs
against the pages-publish contract (exit 6 = "upload or HTTP failure", a broad bucket, not a 404-specific code — confirmed in
README.md#L496-L497).

Checked:

• The retry only fires when replacing is true, the spawn itself didn't error, and status === 6 — a refusal (exit 4), a spawn failure, or a non-replace (no --id) first call are correctly excluded.
• After a retry, the rest of the flow (JSON parse, sidecar write, stale-delete, public-landing guard) runs unchanged against the retried run, so a failed retry still surfaces as exit 1 and the sidecar is left untouched (old id preserved).
• The new fake pages-publish in the test file only applies FAKE_REPLACE_EXIT to calls carrying --id, falling back to FAKE_EXIT otherwise — traced both new tests' call sequences against this and the exit codes line up with the assertions (success-after-retry, refusal-not-retried, create-after-retry-also-fails, first-publish-not-retried).
• Docs (README.md, CHANGELOG.md) and the changelog fragment accurately describe the new behavior and don't restate anything beyond what the code does.

No correctness, maintainability, or test-coverage issues found. The author's noted tradeoff (a transient replace failure can move the page to a new URL) and the open question about delete-of-already-deleted-id are both called out honestly in the PR body and are out of scope for this lane to second-guess — nothing here needs to block.

Author-claimed, unverified (not independently run): the node --test counts, affected-tests.sh, check-changelog-fragments.sh/check-changelog-parity.sh, markdownlint-cli2, and sync-shared-copies.sh results listed in the PR body.

claude-lane-summary: lane=claude-review sha=773c4e52cb914403c9097fda1585832da66c3ed9 run=38347745660

[github-actions]

claude-security-review has reviewed this pull request through 773c4e5; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through 773c4e5; a later push is reviewed from there.