fix(selfhost): use own-property check for retired config-lint fields - #3379
Merged
JSONbored merged 1 commit intoJul 5, 2026
Merged
Conversation
unknownTopLevelWarnings classified retired vs unknown top-level manifest fields with `key in RETIRED_FIELD_MIGRATION_WARNINGS`, which walks the prototype chain. A manifest field named like an Object.prototype member (constructor, toString, hasOwnProperty, valueOf, ...) therefore tested true for the inherited property and resolved to the prototype's function instead of a real warning string — corrupting the string[] result (the function serializes to null over JSON) and suppressing the genuine unknown-field warning for that suspicious key. Use Object.prototype.hasOwnProperty.call, matching the sibling recognizedFieldsFor in the same file. Adds a regression test for a constructor-named field.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3379 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 283 283
Lines 30573 30574 +1
Branches 11138 11138
=======================================
+ Hits 28705 28706 +1
Misses 1211 1211
Partials 657 657
🚀 New features to boost your workflow:
|
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-05 06:27:04 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 3 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
This was referenced Jul 5, 2026
JSONbored
approved these changes
Jul 5, 2026
22 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
…ync the example template Two failures from the #9569 manifest block, both mine. 1. The unknown-top-level-field validator never learned about `publicProof`, so every manifest carrying it warned "Manifest contains unknown top-level field: publicProof." That was invisible on the first pass and appeared on every LATER one, because the first pass parses a manifest with no such key while later passes reload the persisted snapshot -- which my loader change now serializes the field into. The warning lands in the published review comment, so an unchanged PR got a fresh comment PATCH on every regate sweep: exactly the #3379 churn that test exists to prevent, reintroduced by a field the writer knew about and the reader did not. Found by instrumenting the test's PATCH interception to diff the two comment bodies rather than guessing at the cause; the added line named itself. 2. config/examples/loopover.full.yml must mirror .loopover.yml.example from "WHERE IT LIVES" onward, and I documented the block in only one of the two. Verified against origin/main first to confirm both were regressions from this branch rather than pre-existing.
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
…ow the gap exposed Codecov flagged 8 uncovered changed lines across focus-manifest.ts and routes.ts. I had measured coverage on proof-summary.ts and proof-badge.ts only, and never on the two files the manifest block and the routes actually touched -- so the gap was in my own verification, not just the tests. Closing it turned up a real defect rather than only missing assertions: loadProofPageRepoOverride used `.catch()` on the injected manifest loader, so a loader throwing SYNCHRONOUSLY (a driver-level failure before it ever returns a promise) skipped the handler entirely and would have escaped to the route -- 503ing a public page over a manifest read that is supposed to be optional. That is the same defect this file already had in loadProofSummary's section reads, which I fixed there and then reintroduced here. Now a real try/catch, with a regression test using a synchronously-throwing loader. Coverage: - parsePublicProofConfig / publicProofConfigToJson: explicit on/off, a present-but-empty block (present-but-false, which the resolver keys on), absence, three non-mapping shapes warning rather than throwing, and a snapshot round-trip. - A regression test asserting publicProof is a KNOWN top-level field, so the writer/reader split behind the #3379 regate churn cannot return. - The two route 503 arms are unreachable today (every inner read is individually fail-safe), so they are excluded with the house v8 pragma and a note on why they are kept: a future unguarded read should degrade to 503 rather than 500 on an unauthenticated public route. The badge arm uses ignore start/stop -- `next 2` miscounts across a multi-line comment and left the return uncovered. All three changed files now report zero uncovered changed lines.
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
…#9569) (#9608) * feat(proof): public per-repo proof summary, endpoint and README badge (#9569) The shareable, unauthenticated twin of the in-app trust panel. One composition serves both, so the public page and #9193's panel cannot disagree about a figure -- which is the property the page exists to demonstrate. THE PRIVACY BOUNDARY IS STRUCTURAL. Every field is built by NAMING it, never by filtering a wider object. A blocklist has to anticipate every field a future upstream type might grow and silently leaks the one it did not; an allowlisted shape cannot leak a field nobody wrote down. Tested by feeding hostile records carrying hotkey/wallet/reward/trust-score/private- rank and asserting none of it reaches the serialized page -- while the named fields do, so the test proves allowlisting rather than an empty object. NEVER A BARE SCALAR. Any accuracy figure carries its coverage and a Wilson interval; below a 20-decision floor there is no rate at all, only an explicit insufficient_data state that still publishes the count. A perfect record over 19 decisions must not render as 100%. Wilson rather than Wald because a gate metric lives near p->1, exactly where Wald claims impossible certainty. HONEST BOUNDARY STATES. An empty ledger is `empty`, not `verified` -- different claims. A failed read is `unavailable`, not `broken`, which would accuse the operator of tampering. A FAILED anchor attempt is not an anchor: the public attempt log is where failures are legible, and presenting one here would claim corroboration that does not exist. The verification-contract boundary statement travels IN the payload, so a screenshot or embed cannot shed it the way a footer caption can. The badge reports the LEDGER's state rather than an accuracy percentage: a badge is a one-glance claim, and an accuracy number without the interval that makes it honest does not fit in one. Disabled and errored both render a neutral SVG -- a broken image in a README is worse than an honest "unavailable". DECISION (requirement 6), recorded beside the code that implements it: the page is opt-OUT per repo, default ON once the operator's fleet-wide flag (default OFF) is on. Every figure is already publicly fetchable through the ledger-verify / anchors / decision-record endpoints, so gating a page over it would add friction without privacy. The per-repo switch still exists because a page is a different artifact from an API -- discoverable, linkable, and it markets a repo's numbers whether or not the maintainer wants that. A repo can opt out but cannot opt IN when the operator has not, which keeps the fleet switch a real switch. Found and fixed while testing: `DB.prepare()` throws SYNCHRONOUSLY on a driver-level failure, so the `.catch()` chain never ran and a D1 outage would have 503'd the whole public page instead of degrading. Each section is now a real try/catch, which is the difference between the fail-safe-per-section contract being documented and being true. Backend half of #9569; the /proof/:owner/:repo UI route renders this payload and lands separately. * fix(proof): actually wire the per-repo opt-out the routes only claimed to honor (#9569) Review caught the real defect: both handlers called isProofPageEnabledForRepo(c.env) with no second argument, so the ProofPageRepoOverride documented at length in proof-summary.ts and in the PR body was never loaded or passed. Every repo was effectively opt-out-less once the fleet flag was on -- a gate that is described, typed, and unit-tested as a pure function, but never reachable from the surface it governs. That is the registered-but-unreachable class, and the long comment made it worse rather than better by making it look done. - Adds a real `publicProof:` focus-manifest block (engine parser + toJson + loader snapshot), mirroring `publicStats:`/`ops:`. Precedence is deliberately the opposite of those two: read from the TARGET repo's manifest rather than the operator's self-repo, because the thing being opted out of is that repo's own page. - loadProofPageRepoOverride resolves it, degrading a failed manifest load to "no override" -- a broken manifest never takes a page DOWN, which is the failure direction worth accepting here and is now stated in the doc comment rather than left implicit. - Both routes load the override BEFORE anything else, so a repo that turned its page off does not have its decision records queried to build a summary that will be discarded. - Documents the block in .loopover.yml.example, including the precedence and the opt-out default. Tests that would have caught it: a repo opting out in its manifest now gets 404 from BOTH routes with the fleet flag on, while a different repo in the same fleet still serves 200 (the opt-out is per repo, not a kill switch); explicit opt-in and no-block-at-all both serve; and the resolver is covered across absent/explicit/failing loads. * fix(build): build @loopover/contract in ui:build, unbreaking the Cloudflare Workers build The Workers build for loopover-ui has been failing on every PR since #9521 (merged as #9590) made src/openapi/schemas.ts import @loopover/contract/public-api: Cannot find module '.../node_modules/@loopover/contract/dist/public-api.js' imported from /opt/buildhome/repo/src/openapi/schemas.ts ui:build builds ui-kit and engine, then runs ui:openapi -- but never builds the contract package, so the import resolves to a dist/ that does not exist. CI did not catch it because the GitHub workflow has its own separate "Build contract package" step (ci.yml:361) before the drift checks; the Cloudflare build runs npm run build:cloudflare -> ui:build directly and gets no such step. The two paths had silently diverged. Add @loopover/contract to the same turbo invocation that already builds the engine, so the one script both paths share produces everything ui:openapi imports. Reproduced locally by deleting packages/loopover-contract/dist and running ui:openapi (identical ERR_MODULE_NOT_FOUND), then confirmed the fixed chain builds the package and writes the spec with no drift. * fix(manifest): register publicProof as a known top-level field, and sync the example template Two failures from the #9569 manifest block, both mine. 1. The unknown-top-level-field validator never learned about `publicProof`, so every manifest carrying it warned "Manifest contains unknown top-level field: publicProof." That was invisible on the first pass and appeared on every LATER one, because the first pass parses a manifest with no such key while later passes reload the persisted snapshot -- which my loader change now serializes the field into. The warning lands in the published review comment, so an unchanged PR got a fresh comment PATCH on every regate sweep: exactly the #3379 churn that test exists to prevent, reintroduced by a field the writer knew about and the reader did not. Found by instrumenting the test's PATCH interception to diff the two comment bodies rather than guessing at the cause; the added line named itself. 2. config/examples/loopover.full.yml must mirror .loopover.yml.example from "WHERE IT LIVES" onward, and I documented the block in only one of the two. Verified against origin/main first to confirm both were regressions from this branch rather than pre-existing. * test(proof): close the patch-coverage gaps, and fix a second sync-throw the gap exposed Codecov flagged 8 uncovered changed lines across focus-manifest.ts and routes.ts. I had measured coverage on proof-summary.ts and proof-badge.ts only, and never on the two files the manifest block and the routes actually touched -- so the gap was in my own verification, not just the tests. Closing it turned up a real defect rather than only missing assertions: loadProofPageRepoOverride used `.catch()` on the injected manifest loader, so a loader throwing SYNCHRONOUSLY (a driver-level failure before it ever returns a promise) skipped the handler entirely and would have escaped to the route -- 503ing a public page over a manifest read that is supposed to be optional. That is the same defect this file already had in loadProofSummary's section reads, which I fixed there and then reintroduced here. Now a real try/catch, with a regression test using a synchronously-throwing loader. Coverage: - parsePublicProofConfig / publicProofConfigToJson: explicit on/off, a present-but-empty block (present-but-false, which the resolver keys on), absence, three non-mapping shapes warning rather than throwing, and a snapshot round-trip. - A regression test asserting publicProof is a KNOWN top-level field, so the writer/reader split behind the #3379 regate churn cannot return. - The two route 503 arms are unreachable today (every inner read is individually fail-safe), so they are excluded with the house v8 pragma and a note on why they are kept: a future unguarded read should degrade to 503 rather than 500 on an unauthenticated public route. The badge arm uses ignore start/stop -- `next 2` miscounts across a multi-line comment and left the return uncovered. All three changed files now report zero uncovered changed lines. * refactor(proof): one shared resolver for both surfaces, and delete the unreachable arms Replaces the coverage pragmas with the fix they were papering over. The gate, the read and the outcome now live in ONE resolver (resolveProofPage) that both handlers render. That is not tidiness: the gate previously lived inline in both route bodies and exactly one of them was wired to the per-repo opt-out, which is the defect review caught. A shared resolver makes "the page and the badge agree about whether this repo is published" true by construction instead of by two call sites remembering the same thing. With that in place the two 503 arms were provably unreachable, because loadProofSummary is TOTAL -- every read is wrapped per section, so a failing ledger/anchor/record read degrades to that section's honest neutral state and the page still composes. Rather than excluding dead branches from coverage, the outcome is gone from the type: ProofPageResult is `ok | disabled`. A test asserts the totality directly -- every dependency failing at once, including a DB binding that throws on property access, still resolves to a rendered page in its neutral states. Same treatment for buildProofAccuracy's `!interval` guard: wilsonInterval returns null exactly when there are no trials, which IS the nothing-decided case, so one reachable guard covers both reasons a rate is unpublishable instead of a dead branch behind a pragma. Net: no `v8 ignore` pragmas anywhere in the #9569 code, and zero uncovered changed lines or branches across proof-summary.ts, routes.ts and focus-manifest.ts. --------- Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>
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
2 participants
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
unknownTopLevelWarnings()insrc/selfhost/config-lint.tssplits unknown top-level manifest fields into "retired" (with a migration warning) vs genuinely "unknown", using the JavaScriptinoperator:RETIRED_FIELD_MIGRATION_WARNINGSis a plain object literal, soinwalks the prototype chain:"constructor" in obj,"toString" in obj,"hasOwnProperty" in obj,"valueOf" in obj, etc. are alltrue. A manifest with a field named like anObject.prototypemember is therefore misclassified as retired, andRETIRED_FIELD_MIGRATION_WARNINGS["constructor"]resolves to the inheritedObjectfunction, which is pushed into a declaredstring[]. BecauseJSON.stringifydrops function values, that warning serializes tonullover any API — and the genuine"Manifest contains unknown top-level field: constructor."warning is silently suppressed, so the suspicious unknown field disappears from operator-facing output.The sibling
recognizedFieldsForin the same file already does this correctly withObject.prototype.hasOwnProperty.call(...); this call site was the inconsistent one. The fix uses an own-property check. Adds a regression test that lints aconstructor:field and asserts the correct unknown-field warning plus that every warning is a string.No linked issue: issue creation on this repo is restricted to collaborators, and this is a small, self-contained correctness fix (own-property check, matching the sibling in the same file) whose rationale is self-evident from the diff.
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
npm run test:ciaggregate ran green plusnpm audit --audit-level=moderate(0 vulnerabilities). The changed lines are exercised by the existing retired/unknown-field tests plus the new constructor-named-field regression test (which fails on the oldinoperator and passes withhasOwnProperty), socodecov/patchis 100% for the diff.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.Pure config-lint logic; it makes operator-facing manifest linting correct for prototype-named fields and exposes no secrets. No auth/CORS/session surface, no API/OpenAPI shape change, no UI.
Notes