Repository navigation
Document and harden the Difficulty Ladder peer contract (#13) - #17
Conversation
The difficulty glasses are Section Map's only cross-plugin dependency, and neither the README nor the manifest said so. Pin the contract in screen.js, CLAUDE.md and the README, and harden the handshake itself. - Retry the capability check on every 200ms poller tick instead of deciding once per player-screen mount. The old one-shot check relied on the Host loading plugins alphabetically; core's session.js says plugins load ASYNCHRONOUSLY "in whatever order the race settles", so a late or reloaded Difficulty Ladder left the feature unsubscribed for the rest of the session. There is no readiness event to subscribe to: the peer sets its marker at top-level script execution and declares no capability domain. - Require window._ddCapabilities.sectionDifficulty === true rather than the mere existence of that global. - Normalize payloads before rendering: drop entries whose fillPercentage is not a finite number (the peer's arithmetic can produce NaN, which sailed through the old typeof === 'number' check and reached the style attribute as height:NaN%), and ignore a payload container that isn't an object so the last good state survives. A well-formed map with nothing renderable still clears the glasses. - Allowlist glassSize against SM_GLASS_SIZES: it is the one payload field that reaches markup unescaped, as data-size. - Publish Difficulty Ladder v0.12.0 as the payload floor. The issue proposed v0.9.13, but that version was never released; the peer's own README and CHANGELOG call v0.12.0 the floor (its 0.12.0 entry is where difficulty_ladder.sections.v2 is documented; the event itself shipped in v0.2.0). Document it as a floor rather than a check: the capability marker carries no version and predates v0.2.0, so a below-floor peer still renders, it just computes fillPercentage with the pre-0.9.11 formula and can disagree with Difficulty Ladder's own HUD. - Declare the optional peer in plugin.json's capabilities block (roles, observes, compatibility degrade-noop), which is the manifest surface the spec defines for cross-plugin participation: there is no dependency key. - Add 12 peer-contract tests covering an absent provider, a foreign _ddCapabilities global, a late-loading provider, v0.12.0-shaped and current (v3-advertising) payloads, malformed events, unreadable entries, glassSize markup, show/hide re-subscription, and the no-unsubscribe-handle retry path. v3 (difficulty:sections-updated-v3) is deliberately left unconsumed; #14 owns that rework.
The retry itself was already right, but the rationale written for it was not: it claimed core does not order plugin loads, citing session.js's note that "plugins load ASYNCHRONOUSLY" (which is about the plugin/navigation wrapper chain, not competing plugin scripts). static/js/plugin-loader.js is more specific, and the real answer is stronger than "ordering isn't guaranteed": - it sorts plugins by `name || id`, ties broken by id, and injects screen.js one at a time awaiting each onload, so with both plugins 'ready' in a single response difficulty_ladder really does sort ahead of section_map; - but it also skips any plugin whose status isn't 'ready', and the backend clears its registry at the start of load_plugins() and repopulates it incrementally, so a response can be partial; - and it refetches on `plugin-registered` startup events. So a peer mid-install lands on a later refetch, after our script body ran, and our script does not run again either (loadedScripts guard). The tick remains the only retry - only the explanation of why it is needed changes.
|
The failing check was CodeFactor (the only check run in suite 101065616196) with a single annotation: The spread was a defensive copy of the handler list in the test double's Commit
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 high |
🟢 Metrics 14 complexity · 19 duplication
Metric Results Complexity 14 Duplication 19
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
Addressed both threads from the Handler leak on hide/show (fixed, pushed as Capability-gate strictness (verified, no code change) — I fetched the peer's Both threads resolved; a fresh re-read of the review surfaced no additional comments. Task list (8/8 completed)
|
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Notes
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit fbe601c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit fbe601c)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Notes
Fix these issues in Kilo Cloud Previous review (commit 58cb226)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Notes
Fix these issues in Kilo Cloud Previous review (commit 81a91bf)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Notes
Reviewed by free · Input: 46.3K · Output: 2.8K · Cached: 171.3K |
CodeFactor flagged unicorn/no-useless-spread on fire(): it copied the handler list before iterating. Make the fake bus copy-on-write in on() instead, so fire() can iterate the live list directly -- same dispatch semantics, no throwaway array.
|
@pullfrog merge if all Codacy findings are false positives |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Codacy's three
action_requiredannotations were triaged individually and all three are false positives, so this is mergeable as-is. One stale comment inline.The merge itself could not be executed from this run — the token lacks the scope. Details below.
Reviewed changes — the Codacy "Object Injection" trio on screen.js (action_required, 3 new issues), triaged against the full diff, plus the whole screen.js peer-contract diff and its 12 new tests. Suite re-run locally: 35/35 pass.
screen.js:128—const entry = raw[key](Variable Assigned to Object Injection Sink).keyis bound byfor (const key of Object.keys(raw)), so the key space is exactly the payload's own enumerable keys — there is no attacker-chosen index. The value read is type-checked on the next line (!entry || typeof entry !== 'object') and value-checked on the one after (Number.isFinite(entry.fillPercentage)), so nothing unvalidated survives the loop body.screen.js:131—normalized[key] = entry(Generic Object Injection Sink). The write target is a{}created two lines earlier in the same call and never shared or reused as a prototype. I confirmed the__proto__edge case empirically: a payload key of__proto__only re-points that local object's own prototype —Object.prototypestays clean (Object.prototype.fillPercentage === undefined), no global is polluted, and nothing reads_smSectionDifficulty's prototype chain._smGetSectionDifficultylooks up a numeric loop index only (screen.js:140), and every consumer re-checks the entry before rendering.screen.js:503—const glassStyle = SM_GLASS_SIZES[glassSize](Variable Assigned to Object Injection Sink). Provably bounded:glassSizecomes from_smGlassSize()one line above, which returns the peer string only whenObject.prototype.hasOwnProperty.call(SM_GLASS_SIZES, size)confirms it, else the literal'medium'. I verified the allowlist rejects__proto__,constructorandtoString, so the index is one ofsmall|medium|largeand the value is one of three constant CSS strings. This is also the inverse of a real finding — the line it replaces wassizeStyles[difficulty.glassSize] || sizeStyles.medium, an unguarded peer-string index into an object literal that would have resolvedsizeStyles["constructor"]toObject. Hoisting the table to module scope and allowlisting through_smGlassSize()is the fix; the rule can't see the guard.
Worth stating once, since "Security: 3 high" reads worse than it is: the payload originates from difficulty_ladder, a plugin the user installed into the same page and origin, running with identical privileges. There is no trust boundary between the two plugins, so peer-supplied keys are not attacker-supplied. The surface that was worth closing — glassSize reaching data-size unescaped — is closed by the allowlist this PR adds, and Number.isFinite stops the peer's NaN arithmetic from painting height:NaN%.
The new tests are real coverage rather than fixtures that pass either way: the glassSize test asserts markup containment against "><script>alert(1)</script>, the unreadable-entry test asserts Object.keys(...) is [] for a payload of five unreadable entries, the retry test asserts registrations === 1 across five ticks, and the normalizer test pins null and {} as distinct return values including the fillPercentage: 0 case that a truthiness check would have dropped.
Everything else on the PR is green: ci / js, ci / python, CodeQL, CodeFactor, CodeRabbit, and all three compliance checks pass, and both Kilo threads are resolved.
Merge blocked by token scope. The app installation behind this run holds contents:read, issues:write, and pull_requests:write on this repo only. Merging writes the base branch, so both merge paths refuse:
GraphQL: Resource not accessible by integration (mergePullRequest)
HTTP 403 {"message":"Resource not accessible by integration"} PUT /repos/.../pulls/17/merge
{"message":"Resource not accessible by integration"} enablePullRequestAutoMerge
viewerCanMergeAsAdmin is false for this token, and squash-merge via REST returns the same 403, as does enabling auto-merge. A squash merge at fbe601c is all that's needed to land it.
ℹ️ Nitpicks
- The comment in
tests/screen.test.jsdescribing whyfakeEventBusreplaces its handler list saysfire()iterates the live list and a handler may (un)subscribe while it runs. Sinceon()and the disposer bothseta new array rather than mutating, thefor...ofinfire()iterates the snapshot captured at entry — a handler that subscribes mid-dispatch is not picked up by the dispatch in progress. Behaviour is correct either way; only the stated rationale is off.
One follow-up worth deciding separately: because Codacy gates on new issues, the two Object Injection annotations at screen.js:128/:131 will recur on any future plugin payload this file consumes. Silencing them in Codacy rather than in code is the right call — no change to this two-line validation removes them without contorting it into Object.fromEntries gymnastics, and the code as written is the correct one.
big-pickle (free) ｜ 𝕏
|
Addressed the single open thread from review 5416996375. The nitpick was right: the Nothing was left unaddressed — the re-read of the review at
|
fire() reads the handler list once when the loop starts, and both on() and the disposer replace the array instead of mutating it, so the iteration is over the snapshot captured at entry. The previous wording claimed it iterated the live list.
|
@pullfrog please check issues were closed |
|
Checked every issue in the repo against what actually landed, and closed the one that was genuinely done. #13 — closed as completed. It was still open because PR #17 said
The closing comment records the two items deliberately not taken on — consuming the v3 payload, and adding a #14 — left open, deliberately. Its eight remediation items are untouched; the merged change hardened the existing v2 glass contract, whereas #14 is the rework that replaces it with a semantic tier rail. I added a short comment noting where the #13/#14 boundary sits, plus the two things worth knowing before scheduling it: the peer-side prerequisite (feedback-plugin-difficulty-ladder#131, since Difficulty Ladder has to stop emitting Repo state is now 1 closed (#13), 1 open (#14), and #1 / #5 already completed. No code changes, nothing to push. Task list (5/5 completed)
|
Implements #13. The per-section difficulty glass is Section Map's only cross-plugin dependency, and neither the README nor the manifest documented it. This pins the contract in
screen.js/CLAUDE.md/README, hardens the handshake, and declares the optional peer inplugin.json.What changed
Runtime hardening (
screen.js)_smEnsureDifficultySubscription()replaces the one-shot capability check and now runs from every 200 ms_smUpdate()tick as well as on mount. See the rationale below — this is the fix with the widest blast radius._smIsDynamicDifficultyAvailable()requireswindow._ddCapabilities.sectionDifficulty === truerather than the mere existence of that global, so a foreign user of_ddCapabilitiescan't unlock a subscription whose payload we can't render._smNormalizeSectionDifficulties(): drops entries without a finite numericfillPercentageand returnsnull("ignore this event") distinctly from{}("clear the glasses"). Two things this fixes — the peer's arithmetic yieldsNaNwhen mastery or phrase difficulty isn't a number, andNaNistypeof 'number', so it reached the style attribute asheight:NaN%with aDifficulty: NaN%title; and a payload container that isn't an object no longer wipes good state.glassSizeis allowlisted against a newSM_GLASS_SIZEStable via_smGlassSize()before either render path interpolates it — it's the only payload field that reaches markup unescaped, asdata-size._smRenderGlassFilling()/_smUpdateDifficultyFills()now useNumber.isFiniteas defence in depth rather than relying solely on the normalizer.feedBack.on()has been called we mark it subscribed even if the bus returned nothing, so a bus without a disposer can't leak one handler per tick.Docs
CLAUDE.mdgains the peer-contract rules, including two that exist specifically to stop a future refactor from reintroducing the one-shot check.Manifest
version→1.3.0(the version is the cache-buster for served JS/CSS, so this is required for the fix to reach users).capabilities.difficulty-ladderdeclaration:roles: ["observer"],observes: ["difficulty:sections-updated"],compatibility: "degrade-noop",safety: "safe". The plugin schema permits additional properties and defines no dependency key, so this is the surface the spec provides for cross-plugin participation. Validated against the publishedplugin.schema.json.Tests —
node tests/screen.test.js: 35 pass, up from 23. New coverage: absent provider, a foreign_ddCapabilitiesglobal, a provider that appears after mount, a v0.12.0-shaped payload and a current one (a provider advertisingsectionsSchemav3), events with no/unusable detail, unreadable payload entries,glassSizemarkup containment, show/hide re-subscription, retry-until-bus-available, and the no-unsubscribe-handle path.Notes for the reviewer
difficulty_ladder.sections.v2payload shape at 0.12.0. v0.9.13 appears never to have been released, and the peer's own README/CHANGELOG call v0.12.0 the floor. It is documented as a floor and deliberately not enforced: the capability marker carries no version and predates v0.2.0, so a below-floor peer passes the check and still renders — with the pre-v0.9.11 formula, which can disagree with the peer's own HUD. That's stated in the README rather than papered over.static/js/plugin-loader.jssorts byname || id(ties by id) and injectsscreen.jsone at a time awaiting eachonload, so with both pluginsreadyin one responsedifficulty_ladderdoes sort first and a one-shot check happened to work. That is not a guarantee: the loader skips plugins whose status isn'tready, refetches onplugin-registeredstartup events, and the backend clears its registry at the start ofload_plugins()and repopulates it incrementally — so responses can be partial. A mid-install peer then arrives on a later refetch, after our script body has run; our script doesn't re-run either (the loader'sloadedScriptsguard), so the tick is the only retry. An earlier commit in this branch citedsession.js's "plugins load ASYNCHRONOUSLY" note for this, which is about the wrapper chain rather than competing plugin scripts; the second commit corrects it.difficulty:sections-updated-v3is a separate, render-neutral payload emitted alongside v2; the peer's transition policy exists so an un-upgraded Section Map keeps working on the frozen v2 event. Replace section glasses with the shared difficulty tier rail #14 owns that rework, so nothing here sniffsschemato "upgrade".standardskey to the capabilities block.plugin-runtime-idempotent.v1looks defensible (the idempotency guard is real and CI-checked), but I can't verify the full declaration contract from the spec alone, and it isn't what this issue asks for. Worth a separate issue if someone wants to make that claim.🤖 Generated with Pullfrog
big-pickle(free) ｜ 𝕏