Skip to content

fix: refuse challenge remove while a community's settings.challenges still uses the package - #177

Merged
Rinse12 merged 3 commits into
masterfrom
test/challenge-remove-in-use-repro
Oct 20, 2026
Merged

Rinse12 merged 3 commits into
masterfrom
test/challenge-remove-in-use-repro

Conversation

@Rinse12

@Rinse12 Rinse12 commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Closes #176

bitsocial challenge remove deleted a package even while a community's settings.challenges still named it. That's worse than an error on challenge requests. A running community keeps advertising the challenge but rejects every publication, and on its next start (including a daemon restart) start() throws and the community stays offline.

Changes

  • challenge remove refuses an in-use package, the same way page-sort remove does. It asks the daemon at --pkcRpcUrl which communities name the package, running or stopped, and lists each one as <address> settings.challenges[i]. Only name entries count; an entry with a path loads that file instead.
  • Built-ins: a package named after a pkc-js built-in (for example question) is refused too. The message says that removal would fall back to the built-in challenge of the same name.
  • --force skips the check without contacting the daemon, then warns about what happens to communities that still name the package. page-sort remove --force now prints its own warning too.
  • Check order in both commands: validate the name, then confirm it's installed (locally), then check usage through the daemon, then remove and reload. A misspelled name now says "not installed" instead of "could not check".
  • Shared code: collectPackageUsages, assertPackageInstalled and isBuiltInPackageName in package-registry.ts, plus findChallengeUsages in challenge-utils.ts. page-sort remove now uses the shared scan too.
  • Docs: the "Removing" sections in docs/challenges.md and docs/page-sorts.md, and the README command reference.

Tests

  • e2e (challenge-integration.test.ts, real daemon): the refusal test that reproduced the bug now passes. A second test pins the runtime effect of a missing package (every publication rejected, start() throws) and links Unregistered challenge name: opaque Zod error, and start() throws instead of reporting pkcprotocol/pkc-js#428.
  • Unit (challenge.test.ts, fake RPC client): lists usages and skips path entries, removes when unused, refuses a built-in shadow with the fallback note, refuses when the daemon is unreachable, --force warns (plain and built-in wording), and "not installed" is reported before contacting the daemon.
  • page-sort.test.ts: the new check order and the --force warning.
  • command-completion-time.test.ts: challenge remove runs there with no daemon, so it now passes --force.

The full npm run test:cli passes: 409 passed, 1 skipped.

Upstream

pkcprotocol/pkc-js#428 is fixed by pkcprotocol/pkc-js#429. That PR names the error (ERR_CHALLENGE_NOT_REGISTERED, and the publisher's reason says challenge "x" is not registered on this node), but deliberately keeps start() refusing: it throws ERR_COMMUNITY_CHALLENGES_FAILED_TO_LOAD before touching the network. A removed in-use package still takes the community offline, so the refusal here stays necessary.

When pkc-js is bumped to a release that includes #429, update the e2e test's start() assertion (it currently matches the old Zod message) and optionally tighten the reason assertion. The test already handles the error event that #429 adds for a running community.

Summary by CodeRabbit

  • New Features

    • Challenge removal now checks all community settings and refuses removal when the challenge is still referenced.
    • Added --force support to bypass usage checks, with warnings about affected communities.
    • Forced removal of built-in-named challenges restores the built-in challenge where applicable.
    • Page-sort removal now verifies local installation before checking usage and warns when forced.
  • Documentation

    • Updated challenge and page-sort removal guidance with current safeguards, force behavior, and examples.

… uses

The refusal test fails until challenge remove checks settings.challenges.
The runtime test pins what that costs: every publication is rejected and
the community cannot start again.
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 47a6993e-cb79-4f74-b715-be9f1ab8b044
📥 Commits

Reviewing files that changed from the base of the PR and between 9a6b6ff and af35719.

📒 Files selected for processing: 1
  • test/cli/challenge-integration.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Challenge removal now checks community settings before deleting a referenced package unless --force is used. Shared helpers validate installations and collect usage locations. Page-sort removal uses the same usage collection and checks installation before daemon access. Tests and documentation cover these behaviors.

Changes: Package removal checks

Layer / File(s) Summary
Shared usage and package helpers
src/common-utils/package-registry.ts, src/challenge-packages/challenge-utils.ts
Added package installation validation, community usage collection, built-in name detection, and challenge settings usage detection.
Challenge removal guard and force behavior
src/cli/commands/challenge/remove.ts, test/cli/challenge*.test.ts, test/cli/command-completion-time.test.ts, README.md, docs/challenges.md
Challenge removal checks matching settings.challenges entries and reports their locations. Without --force, daemon failures or active usages prevent removal. Forced removal skips the check and reports consequences. Tests cover built-in shadowing and failures after package deletion.
Page-sort removal validation and usage check
src/cli/commands/page-sort/remove.ts, test/cli/page-sort.test.ts, README.md, docs/page-sorts.md
Page-sort removal checks installation before contacting the daemon. Forced removal skips the usage check and warns about remaining references.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant ChallengeRemove
  participant PackageRegistry
  participant Daemon
  User->>ChallengeRemove: Remove challenge package
  ChallengeRemove->>PackageRegistry: Validate installation and built-in name
  alt Force is not set
    ChallengeRemove->>Daemon: Inspect community challenge settings
    Daemon-->>ChallengeRemove: Community settings
    ChallengeRemove->>ChallengeRemove: Find matching name references
  end
  ChallengeRemove->>PackageRegistry: Remove package
  ChallengeRemove->>Daemon: Reload daemon
Loading
Merge Risk: ⚪ Minimal · up to af357

No issue requiring a change before merge was identified in the selected integration-test update.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary change: refusing challenge removal while a community setting references the package.
Linked Issues check Passed Issue #176 requires an in-use check before challenge removal, usage paths, --force, refusal when the daemon is unreachable, tests, and documentation. The reviewed changes validate and install-check …
Out of Scope Changes check Passed The changes remain within issue #176. Shared package-usage and installation helpers support the requested generalization. The page-sort remove updates apply the shared checks and force-warning behav…
Docstring Coverage Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 8 files.
Full details: Linked Issues check

Explanation

Issue #176 requires an in-use check before challenge removal, usage paths, --force, refusal when the daemon is unreachable, tests, and documentation. The reviewed changes validate and install-check the package first, scan running and stopped communities through the daemon, ignore entries with path, list settings.challenges[i], refuse removal, and allow forced removal with warnings. Unit and integration tests cover refusal, daemon failure, force behavior, check order, and the runtime failure after forced removal. The challenge documentation describes the new behavior.

Full details: Out of Scope Changes check

Explanation

The changes remain within issue #176. Shared package-usage and installation helpers support the requested generalization. The page-sort remove updates apply the shared checks and force-warning behavior consistently. Documentation and test changes support the implementation. No unrelated product change is demonstrated.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

test/cli/challenge-integration.test.ts

Oops! Something went wrong! :(

ESLint: 8.27.0

Error: ESLint configuration in --config » eslint-config-oclif is invalid:

  • Unexpected top-level property "__esModule".

Referenced from: /.eslintrc
at ConfigValidator.validateConfigSchema (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2156:19)
at ConfigArrayFactory._normalizeConfigData (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2998:19)
at ConfigArrayFactory._loadConfigData (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2963:21)
at ConfigArrayFactory._loadExtendedShareableConfig (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3264:21)
at ConfigArrayFactory._loadExtends (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3135:25)
at ConfigArrayFactory._normalizeObjectConfigDataBody (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3074:25)
at _normalizeObjectConfigDataBody.next ()
at ConfigArrayFactory._normalizeObjectConfigData (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3019:20)
at _normalizeObjectConfigData.next ()
at ConfigArrayFactory.loadFile (/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2829:16)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…still uses the package

challenge remove now mirrors page-sort remove: it asks the daemon which
communities name the package and refuses, listing each entry. A package
that shadows a pkc-js built-in is refused too, with a note that removal
falls back to the built-in. --force skips the check and warns instead.

Both remove commands now confirm the package is installed before
contacting the daemon, and share the usage scan through
collectPackageUsages in package-registry.ts.
@Rinse12 Rinse12 changed the title test: reproduce challenge remove deleting a package a community still uses (#176) fix: refuse challenge remove while a community's settings.challenges still uses the package Oct 20, 2026
@Rinse12
Rinse12 marked this pull request as ready for review October 20, 2026 09:22
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

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-20T09:24:39.272335Z 9a6b6ff 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.

@Rinse12
Rinse12 merged commit 63b1045 into master Oct 20, 2026
4 checks passed
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.

challenge remove: refuse while a community's settings.challenges still uses the package

1 participant