feat(challenges): {success, error, reason, comment?, commentUpdate?} result shape (#88)#99
Merged
Merged
Conversation
…result shape (#88) Replaces the previously proposed `extraProps` bag with a typed shape on the challenge result. Aggregated across successful challenges (last-wins per key): - `comment` spread last into the IPFS-bound CommentIpfs (becomes part of the CID) - `commentUpdate` persisted on a new `challengeCommentUpdate` JSON column on the comments table; seeded into queryCalculatedCommentUpdate with lowest priority so any mod-published commentModeration overrides matching fields per-field Adds an override guard: throws PKCError if a challenge tries to override a key in CommentSignedPropertyNames (would invalidate the author's signature) or a key in the new CommentUpdateChallengeReservedFieldNames list (community-computed fields like counts, cid, updatedAt). Misconfigured challenges fail loudly via the existing try/catch in getChallengeVerification. The first commentUpdate (encrypted with the verification message) carries the challenge-supplied keys by computing signedPropertyNames at sign time from the actual object keys. DB_VERSION bumped 39 → 40 (additive column, NULL for legacy rows).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional comment/commentUpdate/reason to ChallengeResult; enforces override guards; aggregates extras across challenges; threads aggregates through signing, publishing, and storage; persists challengeCommentUpdate in DB with a v39→v40 migration; and adds tests covering aggregation, overrides, and migration. ChangesChallenge Extras Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…tests DB_VERSION was bumped 39 → 40 in this branch; two pre-existing migration tests assert that the migrated DB lands at the latest version with a hardcoded literal.
Adds coverage for the driving use case where a challenge with pendingApproval: true succeeds and attaches a rationale on commentUpdate.reason, so the mod reviewing the pending queue can see why the challenge sent the comment there.
…author Pins the author-side end of the new commentUpdate-extras flow: - signCommentUpdateForChallengeVerification puts challenge-supplied keys (reason, countryCode, etc.) into signature.signedPropertyNames so the community's signature actually covers them - JSON roundtrip (matches what the encrypt/decrypt boundary does) preserves the reason field
…ason
Replaces the lighter sign+JSON-roundtrip test with a real end-to-end test in the
existing pending-approval modqueue suite: registers a custom challenge on
pkc.settings.challenges that returns {success: true, commentUpdate: {reason}},
publishes through the real challenge pipeline, and asserts on the author-side
challengeverification event that:
- challengeVerification.commentUpdate.reason carries the rationale
- signature.signedPropertyNames includes "reason" (community really signed it)
- the Comment instance exposes `comment.reason` via the unknown-props hoist
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/node/community/challenges/index.ts`:
- Around line 62-66: The code currently uses truthy checks like `if
(challengeResult.reason)` before assigning to `agg.aggregatedReason`, which
drops valid empty-string reasons; change these checks to explicit undefined
comparisons (`if (challengeResult.reason !== undefined)`) wherever
`challengeResult.reason` is inspected (including the occurrences around the
early return and the later assignment and the similar block at lines ~807-810)
so empty-string values are preserved and still propagated to
`agg.aggregatedReason`.
In `@test/challenges/result-extras.test.ts`:
- Around line 153-180: The test currently expects getChallengeVerification to
throw when a challenge overrides a comment-signed field; update it to reflect
the new contract where override violations are returned as a failed verification
instead of thrown. Specifically, call getChallengeVerification as before but do
not wrap it in try/catch expecting an exception; instead assert the returned
value has challengeSuccess === false and that the returned reason/code or
failure metadata indicates ERR_CHALLENGE_RESULT_OVERRIDES_COMMENT_SIGNED_FIELD
(reference getChallengeVerification and the error identifier
ERR_CHALLENGE_RESULT_OVERRIDES_COMMENT_SIGNED_FIELD); apply the same change
pattern to the similar assertions around lines 182-203.
In `@test/node/community/challengeCommentUpdate.db.community.test.ts`:
- Line 22: Add a one-line comment immediately above the
describeSkipIfRpc("queryCalculatedCommentUpdate seeded with
challengeCommentUpdate", ...) that explicitly states why this test cannot run
under RPC (e.g., relies on in-process DB seeding/state, uses local-only helpers,
or depends on synchronous access to in-memory fixtures). Reference the test name
"queryCalculatedCommentUpdate seeded with challengeCommentUpdate" and the helper
describeSkipIfRpc so reviewers can see the RPC incompatibility reason at a
glance; keep the comment short and specific to the RPC limitation.
In `@test/node/community/modqueue/pendingapproval.modqueue.community.test.ts`:
- Around line 349-361: Add a clear RPC-skip rationale comment immediately above
the describeSkipIfRpc(...) suite explaining why the test cannot run under RPC
(e.g., it depends on in-process challenge execution, local commentUpdate-extras
flow, or direct access to in-memory modqueue/encryption hooks that RPC does not
provide). Reference the suite name describeSkipIfRpc and the custom challenge
function pendingWithReasonChallenge so readers understand this test exercises
the local getChallenge → commentUpdate roundtrip and therefore must be skipped
for RPC runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69b6b610-229c-416c-95b5-2d1e8bbd392e
📒 Files selected for processing (16)
src/community/schema.tssrc/errors.tssrc/publications/comment/schema.tssrc/runtime/node/community/challenges/index.tssrc/runtime/node/community/challenges/validate-challenge-result.tssrc/runtime/node/community/db-handler.tssrc/runtime/node/community/local-community/challenges.tssrc/runtime/node/community/local-community/publication-store.tssrc/signer/signatures.tssrc/version.tstest/challenges/result-extras.test.tstest/node/community/challengeCommentUpdate.db.community.test.tstest/node/community/modqueue/pendingapproval.modqueue.community.test.tstest/node/community/v29-production.migration.db.community.test.tstest/node/community/v36-to-v37.migration.db.community.test.tstest/node/community/v39-to-v40.migration.db.community.test.ts
Comment on lines
+62
to
+66
| if (challengeResult.reason) agg.aggregatedReason = challengeResult.reason; | ||
| return; | ||
| } | ||
| if (challengeResult.reason) agg.aggregatedReason = challengeResult.reason; | ||
| if (challengeResult.comment) { |
There was a problem hiding this comment.
Preserve empty-string reason values during aggregation.
Using truthy checks drops valid reason: "" values. Use explicit !== undefined checks so all schema-valid reasons propagate consistently.
Suggested fix
- if (!("success" in challengeResult) || challengeResult.success !== true) {
- if (challengeResult.reason) agg.aggregatedReason = challengeResult.reason;
+ if (!("success" in challengeResult) || challengeResult.success !== true) {
+ if (challengeResult.reason !== undefined) agg.aggregatedReason = challengeResult.reason;
return;
}
- if (challengeResult.reason) agg.aggregatedReason = challengeResult.reason;
+ if (challengeResult.reason !== undefined) agg.aggregatedReason = challengeResult.reason;- if (res.aggregatedReason) challengeVerification.aggregatedReason = res.aggregatedReason;
+ if (res.aggregatedReason !== undefined) challengeVerification.aggregatedReason = res.aggregatedReason;Also applies to: 807-810
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/node/community/challenges/index.ts` around lines 62 - 66, The
code currently uses truthy checks like `if (challengeResult.reason)` before
assigning to `agg.aggregatedReason`, which drops valid empty-string reasons;
change these checks to explicit undefined comparisons (`if
(challengeResult.reason !== undefined)`) wherever `challengeResult.reason` is
inspected (including the occurrences around the early return and the later
assignment and the similar block at lines ~807-810) so empty-string values are
preserved and still propagated to `agg.aggregatedReason`.
Comment on lines
+153
to
+180
| it("surfaces failure when a challenge tries to override a comment-signed field", async () => { | ||
| // `content` is part of CommentSignedPropertyNames. getChallengeVerification wraps its inner | ||
| // loops in try/catch and turns a thrown PKCError into `{ challengeSuccess: false, reason }` | ||
| // so misconfigured challenges fail loudly with a descriptive message. | ||
| (mockPkc.settings ??= {}).challenges = { | ||
| "bad-comment": makeExtrasChallenge({ success: true, comment: { content: "spoofed by challenge" } }) | ||
| }; | ||
| const community = { | ||
| settings: { challenges: [{ name: "bad-comment" }] }, | ||
| _pkc: mockPkc | ||
| } as unknown as LocalCommunity; | ||
|
|
||
| let caught: Error | undefined; | ||
| try { | ||
| await getChallengeVerification({ | ||
| challengeRequestMessage, | ||
| community, | ||
| getChallengeAnswers: (async () => []) as GetChallengeAnswers | ||
| }); | ||
| } catch (e) { | ||
| caught = e as Error; | ||
| } | ||
| // The thrown PKCError bubbles out of the inner classification loop. local-community/challenges.ts | ||
| // catches it at the outer caller (`runChallengeExchangeIfNeeded`); here we exercise the inner | ||
| // function directly, so we see the throw. | ||
| expect(caught).to.not.equal(undefined); | ||
| expect((caught as { code?: string })?.code).to.equal("ERR_CHALLENGE_RESULT_OVERRIDES_COMMENT_SIGNED_FIELD"); | ||
| }); |
There was a problem hiding this comment.
Override-guard tests currently enforce a throw path that conflicts with the target contract.
These assertions expect getChallengeVerification to throw, but this PR’s intended behavior is to convert override violations into a failed verification result (non-throwing). Keeping these as throw assertions will lock in the wrong API behavior.
Suggested test-shape adjustment
- let caught: Error | undefined;
- try {
- await getChallengeVerification(...);
- } catch (e) {
- caught = e as Error;
- }
- expect(caught).to.not.equal(undefined);
- expect((caught as { code?: string })?.code).to.equal("ERR_...");
+ const verification = await getChallengeVerification(...);
+ expect(verification.challengeSuccess).to.equal(false);
+ expect(verification.challengeErrors).to.exist;
+ expect(verification.aggregatedReason).to.be.a("string");Also applies to: 182-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/challenges/result-extras.test.ts` around lines 153 - 180, The test
currently expects getChallengeVerification to throw when a challenge overrides a
comment-signed field; update it to reflect the new contract where override
violations are returned as a failed verification instead of thrown.
Specifically, call getChallengeVerification as before but do not wrap it in
try/catch expecting an exception; instead assert the returned value has
challengeSuccess === false and that the returned reason/code or failure metadata
indicates ERR_CHALLENGE_RESULT_OVERRIDES_COMMENT_SIGNED_FIELD (reference
getChallengeVerification and the error identifier
ERR_CHALLENGE_RESULT_OVERRIDES_COMMENT_SIGNED_FIELD); apply the same change
pattern to the similar assertions around lines 182-203.
…Schema `reason` only makes sense on the failure path — a successful challenge has nothing to explain. Remove it from the success branch of ChallengeResultSchema, stop propagating it through accumulateChallengeResultExtras on success, and revert publishChallengeVerification's success path to `reason: undefined`. Failure-side aggregatedReason (challenge-supplied rationale for rejection) stays as-is.
The previous refactor included `aggregatedComment` and `aggregatedCommentUpdate` as explicit `undefined` keys on the success return, which broke a deep-equal assertion that expected those keys to be absent when no challenge populated them. Destructure `aggregatedReason` away from the success branch and spread the rest so undefined fields stay absent.
AGENTS.md requires an explicit reason above describeSkipIfRpc. Added one-line rationales on the two new suites added in this PR (challengeCommentUpdate DB seeding test and the pending-approval commentUpdate.reason roundtrip test).
Lets a challenge attach `commentUpdate.author.community.<key>` (e.g. a country-code badge from an IP geolocation check) without the previous wholesale `author` ban. The guard is now path-aware: - `commentUpdate.author.<key>`: only `community` is permitted; any other key throws ERR_CHALLENGE_RESULT_OVERRIDES_NON_COMMUNITY_AUTHOR_KEY - `commentUpdate.author.community.<key>`: forbidden if `<key>` is in CommunityAuthorChallengeReservedFieldNames (postScore, replyScore, firstCommentTimestamp, lastCommentCid — community-computed; banExpiresAt — mod-settable via commentModeration.author.banExpiresAt), else allowed as lowest-priority extra `flairs` is intentionally not reserved: a challenge may seed `author.community.flairs` with lowest priority, and a mod publishing `commentModeration.author.flairs` overrides it via the existing queryAuthorModEdits path. queryCalculatedCommentUpdate merges `challengeCommentUpdate.author.community.*` underneath the computed authorCommunity, so any mod-set field wins automatically. `CommunityAuthorChallengeReservedFieldNames` is co-located with `CommunityAuthorSchema` and uses `as const satisfies readonly (keyof ...)[]` so renaming a CommunityAuthor field surfaces a stale entry at compile time, matching CommentUpdateChallengeReservedFieldNames.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/publications/comment/schema.ts`:
- Around line 161-162: The comment above the schema about allowed
author.community keys is contradictory: it says "no schema-defined keys under
community" but the reserved-field contract permits at least certain keys (e.g.,
flairs). Update the comment near the schema and the note referencing
validateChallengeResultExtras to explicitly state that top-level author keys
remain disallowed but reserved author.community keys (such as "flairs") are
allowed per the reserved-field contract; mention validateChallengeResultExtras
and the reserved-field contract by name so future maintainers know validation is
enforced separately and which community keys are permitted.
In `@test/node/community/challengeCommentUpdate.db.community.test.ts`:
- Around line 183-197: The test claims "Same author" but currently `insertPost`
generates a unique `authorSignerAddress` per CID, so make both posts share the
same author to actually validate per-comment isolation: update the test to
create a single author object (or a fixed `authorSignerAddress`) and pass that
same author into both `insertPost(...)` calls for `commentA` and `commentB`,
then run `calculate(...)` as before (symbols: insertPost, calculate,
authorSignerAddress) so the two comments truly originate from the same author
while still having different `author.community.countryCode` values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 317f6c9e-df8e-45bb-82b9-9ce5ee6d0c11
📒 Files selected for processing (7)
src/errors.tssrc/publications/comment/schema.tssrc/runtime/node/community/challenges/validate-challenge-result.tssrc/runtime/node/community/db-handler.tssrc/schema/schema.tstest/challenges/result-extras.test.tstest/node/community/challengeCommentUpdate.db.community.test.ts
…are author across per-comment isolation test - schema.ts: fix contradictory comment that claimed no schema-defined `author.community` keys are allowed. `flairs` is intentionally permitted (lowest-priority seed; mod override via commentModeration.author.flairs). - challengeCommentUpdate.db.community.test.ts: the "Same author, two posts" case was generating a distinct authorSignerAddress per CID, so it could pass even if per-comment isolation broke. Add an override param and pass a shared address.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #88. Replaces the originally proposed `extraProps` bag with a typed shape on the challenge result:
```ts
ChallengeResultSchema = z
.object({
success: z.literal(true),
reason: z.string().optional(),
comment: z.looseObject({}).optional(), // partial CommentIpfs
commentUpdate: z.looseObject({}).optional() // partial CommentUpdate
})
.or(z.object({
success: z.literal(false),
error: z.string(),
reason: z.string().optional()
}));
```
Aggregated across successful challenges (last-wins per key):
Override-guard
A new helper `validateChallengeResultExtras` throws a `PKCError` if a challenge sets:
The list is co-located with `CommentUpdateSchema` and uses `as const satisfies readonly (keyof z.infer)[]` so renaming a CommentUpdate field surfaces a stale entry at compile time.
`getChallengeVerification` already wraps the classification loops in try/catch and turns thrown PKCErrors into `{ challengeSuccess: false, reason: "One of the community challenges is misconfigured: …" }`, so misconfigured challenges fail loudly without crashing the community.
First commentUpdate signing
The `CommentUpdateForChallengeVerificationSchema` stays `.strict()` to keep types clean across the existing union; `signCommentUpdateForChallengeVerification` instead computes `signedPropertyNames` dynamically as `[...base, ...extraKeysOnUpdate]` so challenge keys actually land in the signature.
DB migration
`DB_VERSION` bumped 39 → 40 (additive column, NULL for legacy rows). `_copyTable` handles the migration automatically.
Test plan
Out of scope (per plan)
Summary by CodeRabbit
New Features
Bug Fixes
Tests