refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine - #2443
Merged
Merged
Conversation
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 01:58:01 UTC
⏸️ Suggested Action - Manual Review
Review summary Blockers
Nits — 6 non-blocking
Concerns raised — review before merging
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.
|
loopover-orb
Bot
added
gittensor
gittensor:bug
24 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
==========================================
+ Coverage 95.91% 95.93% +0.01%
==========================================
Files 224 225 +1
Lines 25235 25324 +89
Branches 9177 9214 +37
==========================================
+ Hits 24205 24294 +89
Misses 417 417
Partials 613 613
🚀 New features to boost your workflow:
|
…he registry-review engine gittensory is meant to be installed by any self-hosted repo maintainer, not just JSONbored/metagraphed. The RegistryLaneSpec abstraction was already generic, but nothing let a second maintainer actually reach it without editing gittensory's own TypeScript source and redeploying. - Rename runMetagraphedSurfaceGate to runRegistrySurfaceGate; stop re-exporting netuid-verification.ts's Bittensor-only helpers from the generic content-lane barrel (closes #2433). - Add assessAppendedEntry/assessProviderEntry callback fields to RegistryLaneSpec; the orchestrator calls the spec-supplied validators instead of hardcoded imports, so a different registry can supply its own domain validator without touching shared engine code (closes #2434). - Add a contentLane: block to .gittensory.yml (FocusManifestContentLaneConfig) and a resolveRegistryLaneSpec resolver mirroring resolveConvergedFeature's precedence (env kill-switch -> per-repo config -> allowlist default), so a second maintainer's registry repo can activate the deterministic surface lane purely from their own config, with today's zero-config behavior for metagraphed unchanged (closes #2435). - Glob fields (entryFileGlob/providerFileGlob/artifactGlob) are capped at parse time to a safe wildcard count: an adversarial-review pass on this change found the shared glob-to-RegExp compiler is exponential-time on chained wildcards (empirically ~19s at 5 chained wildcards), so the new config surface rejects an over-complex glob before it ever reaches RegExp compilation.
buildRegistryLaneSpecFromConfig already degraded an unregistered validatorId to structural-only gating silently (a legitimate mode for a registry with no validator yet), which made an operator typo (e.g. "metagraph" instead of "metagraphed") indistinguishable from a deliberate choice — no signal reached the maintainer. Add unregisteredValidatorId()/registeredValidatorIds() to spec-resolver.ts and surface a non-blocking advisory finding from evaluateWithSurfaceLane so a bad validatorId shows up directly in the PR comment, naming the offending id and the known registered ids. Also switch the REGISTRY_VALIDATORS lookup to Object.hasOwn instead of bracket-truthiness, so a validatorId matching an inherited Object.prototype key (e.g. "toString") is correctly reported as unregistered rather than silently matching a prototype method.
JSONbored
force-pushed
the
claude/pr-b-content-lane-genericity
branch
from
July 2, 2026 01:00
23e3fcd to
2ca08ee
Compare
…er-long globs Two gaps flagged in evaluateWithSurfaceLane and normalizeOptionalGlob: - A non-allowlisted repo's ONLY way to configure a registry content lane is its own .gittensory.yml. A manifest-load failure was caught as null and silently treated the same as "no contentLane configured" -- letting a registry-submission PR merge unevaluated on nothing more than a transient read blip. Now holds the gate neutral in that specific case (never overriding a real generic hard blocker, which is always preserved). - An over-long contentLane glob (entryFileGlob/providerFileGlob/ artifactGlob) was truncated to MAX_ITEM_LENGTH and still returned, silently compiling a DIFFERENT file-scope pattern than configured. Now rejected outright, matching the function's own doc comment and the established pattern used elsewhere in this file.
* fix(signals): cap globToRegExp wildcard count to prevent ReDoS hardGuardrailGlobs (src/review/guardrail-config.ts) and any future maintainer-supplied glob compiled via globToRegExp are vulnerable to catastrophic backtracking on chained `*` wildcards: 5 chained wildcards against a 300-char adversarial path took ~19 seconds. Cap wildcard count at compile time (MAX_GLOB_WILDCARDS = 6, well above any of the ~10 real guardrail globs today). An over-complex glob is treated as matching every path rather than failing open, since a guardrail's job is to force manual review on uncertainty — mirroring isGuardrailHit's existing "unknown ⇒ treat as a hit" fail-safe direction. * fix(signals): bake the ReDoS wildcard cap into globToRegExp itself The wildcard-count guard previously lived only in matchesAny's wrapper, so any other direct caller of the exported globToRegExp (e.g. content-lane/spec-resolver.ts) could still compile and .test() a pathological glob and hit catastrophic backtracking — the exact gap this PR set out to close. globToRegExp now short-circuits an over-complex glob to a never-matching sentinel regex instead of compiling it, so every caller (present or future, direct or indirect) is protected automatically. "Never matches" (not "matches everything") is the correct default for the general-purpose compiler, since a false "matches everything" would misclassify unrelated files for a non-guardrail caller; matchesAny keeps its own override to the opposite fail direction for guardrail semantics specifically. * fix(signals): count wildcard GROUPS, not raw * characters, for the ReDoS cap The flagged blocker was correct: MAX_GLOB_WILDCARDS=6 let the PR's own motivating example (5 chained wildcards, empirically catastrophic) through uncapped. Lowering the raw-character cap to 2 fixed that but broke a real consumer -- content-lane/spec-resolver.ts's artifactGlob "public/**/*.json" has 3 raw '*' characters (** counts as two) despite being empirically instant even against a 4,000-char adversarial path, because a `**` globstar compiles to a single .* group, not two independent wildcards. Re-benchmarked with the right unit (wildcard GROUPS, where a ** pair is one group): 2 groups stays sub-second even at 32,000 adversarial chars; 3 groups is already dangerous (over 2s at ~4,000 chars for a chained-* shape, over 100ms at ~1,600 chars for a chained-** shape); 4+ groups is catastrophic (35s at 1,614 chars for 4 chained ** groups). Caps MAX_GLOB_WILDCARD_GROUPS at 2 -- the highest value proven safe -- via a new countWildcardGroups that mirrors globToRegExp's own tokenization (consuming a ** pair, and its trailing /, as one group), so both the original flagged case and real 2-group globs like public/**/*.json are handled correctly.
…arsing and compilation normalizeOptionalGlob (focus-manifest.ts) capped contentLane globs at 3 raw '*' characters, but globToRegExp (change-guardrail.ts) rejects any glob with more than 2 wildcard GROUPS (a '**' pair counts as one group, not two) by compiling it to NEVER_MATCHES. A glob like "a*b*c*.json" (3 groups, no '**' pairs) was accepted as configured but silently could never match any file once compiled -- a content lane that looks active but never fires. Export hasUnsafeWildcardCount from change-guardrail.ts and reuse it directly in normalizeOptionalGlob instead of an independently-counted threshold, so the parser and compiler can never drift apart again.
Closed
12 tasks
12 tasks
12 tasks
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
gittensory is meant to be installed by any self-hosted repo maintainer, not just JSONbored. The content-lane registry-review subsystem was built for and tested against exactly one customer (JSONbored/metagraphed) and its own header comments already promised genericity ("metagraphed is just the FIRST spec... a spec can later be loaded from per-repo
.gittensory.ymlconfig so a new registry needs config, not a code change"). This PR closes that gap:runMetagraphedSurfaceGate→runRegistrySurfaceGate; stop re-exportingnetuid-verification.ts's Bittensor-only helpers from the generic content-lane barrel (they're metagraphed's own domain plumbing, not shared API).assessAppendedEntry/assessProviderEntrycallback fields toRegistryLaneSpec; the orchestrator calls the spec-supplied validators instead of hardcoded imports, so a different registry can supply its own domain validator without touching shared engine code. A spec with no validator configured gets amanualverdict (structural gating still applies) rather than a crash.contentLane:block to.gittensory.yml(FocusManifestContentLaneConfig) and aresolveRegistryLaneSpecresolver mirroringresolveConvergedFeature's existing precedence (env kill-switch → per-repo config → allowlist default), so a second maintainer's registry repo can activate the deterministic surface lane purely from their own config. Today's zero-config behavior for metagraphed is unchanged (falls back toMETAGRAPHED_LANE_SPECvia the existing allowlist).Why one PR for three issues: these were designed and built as one continuous, tightly-coupled sequence (each step's tests/wiring build directly on the previous), and splitting them further after the fact would require redundant rework without additional review value — the same reviewer needs the full arc to evaluate any one piece. Commits reference all three issue numbers.
Security fix included: an adversarial-review pass on this change found that the shared glob-to-RegExp compiler (
change-guardrail.ts'sglobToRegExp, newly exported here for reuse) is exponential-time on adversarial chained-wildcard patterns — empirically verified ~19 seconds at 5 chained wildcards against a 300-char input. The newcontentLane.*Globconfig fields cap wildcard count at parse time (normalizeOptionalGlob,MAX_GLOB_WILDCARDS = 3, empirically verified safe), rejecting an over-complex glob before it ever reaches RegExp compilation. This fix is scoped to the new config surface only — hardening the pre-existinghardGuardrailGlobsconsumer of the same function is a separate follow-up PR (different, safety-critical fail-direction; see that PR for details).Maintainability fix included: an unregistered
contentLane.validatorId(an operator typo, e.g. "metagraph" instead of "metagraphed") previously degraded silently to structural-only gating — indistinguishable from a deliberate no-validator config.spec-resolver.tsnow exposesunregisteredValidatorId/registeredValidatorIds, andevaluateWithSurfaceLanepushes a non-blocking advisory finding naming the bad id and the known registered ids directly into the PR comment. Also switched theREGISTRY_VALIDATORSlookup toObject.hasOwn(was bracket-truthiness) so avalidatorIdmatching an inheritedObject.prototypekey (e.g."toString") is correctly reported as unregistered.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlint(vianpm run test:ci)npm run typechecknpm run test:coveragelocally — 100% statements/branches/functions/lines on every changed line in every touched filenpm run test:workers(vianpm run test:ci)npm run build:mcp(vianpm run test:ci)npm run test:mcp-pack(vianpm run test:ci)npm run ui:openapi:check(vianpm run test:ci) — no API/schema changesnpm run ui:lint(vianpm run test:ci)npm run ui:typecheck(vianpm run test:ci)npm run ui:build(vianpm run test:ci)npm audit --audit-level=moderate— 0 vulnerabilitiesvalidatorIdsurfaces a diagnostic finding (including anObject.prototype-key edge case).Safety
.gittensory.ymlconfig-as-code, not a REST/MCP surface).UI Evidencesection. — N/A, backend-only change, no UI Evidence needed.Notes
contentLanefollows the same no-DB precedence pattern as the other converged features (rag/reputation/unifiedComment/safety), not the DB-backedsettings:/gate:shape — it has no dashboard UI today and doesn't need one for this change.REGISTRY_VALIDATORS) rather than an attempt at a fully generic rules engine — matching the boundary the repo owner set: shared code stays generic, repo-specific richness belongs in each maintainer's own config/skills surface.main; this branch was rebased directly ontomainto drop the now-redundant duplicate commit, so the PR base retargeted automatically and the diff is scoped to just this PR's own changes.