Enforce OSL style with OSL's own lint rules

.github/workflows/test.yml

@@ -23,7 +23,7 @@ jobs:
 
       - name: Install OSL
         env:
-          OSL_COMMIT: 5895982fa61d3e7c9e9c7f819c9e33f645d04f0f
+          OSL_COMMIT: 78deaec4824587cf9d71efcf5c594b521e3ab167
         run: |
           mkdir -p "$HOME/.local/bin"
           git clone --filter=blob:none https://git.rotur.dev/osl/.go /tmp/osl-source

AGENTS.md

@@ -22,7 +22,9 @@ The editor and site depend on every path, method, middleware order, body limit a
 
 ## Checks
 
-`osl test`, `osl lsp check main.osl`, `osl compile main.osl -o main`, and `python3 scripts/check_conventions.py`. The last one rejects code comments and em dashes in the lines you add. CI (`.github/workflows/test.yml`) runs all of them plus the smoke test, with OSL pinned to a commit.
+`osl test`, `osl lsp check main.osl`, `osl compile main.osl -o main`, and `python3 scripts/check_conventions.py`. CI (`.github/workflows/test.yml`) runs all of them plus the smoke test, with OSL pinned to a commit.
+
+OSL style is enforced by the `lint` section of `opal.json`, which OSL's own linter applies in every check, test and compile. It makes comments (`OSL-COMMENT`) and top-level variables without a declared type (`OSL-UNTYPED-GLOBAL`) errors. `OSL-OBJECT-PARAM` and `OSL-LONG-FUNCTION` are warnings for now. The only comments allowed are the `// osl-disable-next-line` and `// osl-disable-file` directives. They must name the codes they turn off and can never turn off `OSL-COMMENT`, which the conventions script checks. The caches that hold whole-collection snapshots stay `auto` behind `// osl-disable-next-line OSL-UNTYPED-GLOBAL`, because a typed `*cache.Cache` converts every value it returns and those values are read on every request. `scripts/check_conventions.py` covers everything else: it rejects code comments in the Go and Python lines you add, and em dashes in the added lines of every text file, OSL included.
 
 ## Working here
 

main.osl

@@ -66,9 +66,6 @@ if !r2Local and appURL == "http://localhost:5627" (
   log "WARNING: APP_URL points to localhost but R2 is configured; thumbnails and pending project.json are served from APP_URL and will be unreachable. Set APP_URL to the public https API base."
 )
 
-// `main storage-report <file>` writes what MistWarp stores for everyone, for
-// the Rotur API's `claw storage-backfill`, and exits. Run it with the API
-// stopped.
 array commandArgs = process.args()
 if commandArgs.len >= 1 and commandArgs[1].toStr() == "storage-report" (
   if commandArgs.len != 2 (

opal.json

@@ -10,5 +10,13 @@
     "routes": ["python3", "scripts/route_dump.py", "."],
     "smoke": ["python3", "scripts/e2e_smoke.py"],
     "conventions": ["python3", "scripts/check_conventions.py"]
+  },
+  "lint": {
+    "rules": {
+      "OSL-COMMENT": "error",
+      "OSL-UNTYPED-GLOBAL": "error",
+      "OSL-OBJECT-PARAM": "warning",
+      "OSL-LONG-FUNCTION": "warning"
+    }
   }
 }

scripts/check_conventions.py

@@ -9,6 +9,7 @@
 TEXT_FILE = re.compile(r"\.(osl|go|py|md|json|yml|yaml|sh|txt)$")
 SKIPPED = [re.compile(r"(^|/)go\.sum$"), re.compile(r"^data/")]
 DIRECTIVE = re.compile(r"^(//|#)\s*(nolint|noqa|go:|lint:|type:\s*ignore)")
+OSL_DISABLE = re.compile(r"^\s*//\s*osl-disable-(?:file|next-line)(?P<codes>(?:[ \t].*)?)$")
 
 
 def git(*args):
@@ -79,7 +80,7 @@ def python_comments(text):
     return found
 
 
-SCANNERS = {".osl": c_style_comments, ".go": c_style_comments, ".py": python_comments}
+SCANNERS = {".go": c_style_comments, ".py": python_comments}
 
 
 def main():
@@ -97,6 +98,9 @@ def main():
         for line, content in added.items():
             if EM_DASH in content and content.strip() not in removed:
                 problems.append(f'{path}:{line}  em dash: use a comma, parentheses or "to"')
+            escape = OSL_DISABLE.match(content) if path.endswith(".osl") else None
+            if escape and (not escape.group("codes").strip() or "OSL-COMMENT" in escape.group("codes")):
+                problems.append(f"{path}:{line}  lint directive: name the codes it turns off, and never OSL-COMMENT")
         scan = next((scanner for suffix, scanner in SCANNERS.items() if path.endswith(suffix)), None)
         if scan is None:
             continue

src/admin/admin.osl

@@ -15,8 +15,9 @@ export {
   unbanUser
 }
 
+// osl-disable-next-line OSL-UNTYPED-GLOBAL
 auto bansCache = cache.new({capacity: 2, ttl: 300000})
-auto adminsCache = cache.new({capacity: 2, ttl: 300000})
+*cache.Cache adminsCache = cache.new({capacity: 2, ttl: 300000})
 
 def loadAdmins() array (
   auto res = readFile(adminsFile)
@@ -64,13 +65,10 @@ def cachedBans() object (
   return data
 )
 
-// Rebuilt whenever the bans file is saved (a ban, an unban, a rename, or the
-// Rotur sync dropping a ban that ended there), and at most 5 minutes old.
 def bannedOwners() object (
   if bansCache.has("owners") (
     return bansCache.get("owners").assert(object)
   )
-  // One rebuild at a time; the rest wait for it and use its result.
   sync.lock("banned-owners")
   defer sync.unlock("banned-owners")
   if bansCache.has("owners") (
@@ -105,7 +103,6 @@ def projectsVisibleTo(array entries,*serve.Context c) array (
   return projectEntriesListed(entries, bannedOwners(), shadowBannedSet(), normalizeUsername(contextString(c, "username")), contextString(c, "userId"), contextBool(c, "isAdmin"))
 )
 
-// For lists that are cached publicly, so never differ for admins.
 def projectsVisibleToEveryone(array entries) array (
   return projectEntriesListed(entries, bannedOwners(), shadowBannedSet(), "", "", false)
 )
@@ -137,23 +134,14 @@ def banReasonFor(string username) string (
   return ""
 )
 
-// MistWarp's bans are also bans from MistWarp's Rotur App, so Rotur enforces
-// them: the person's MistWarp tokens stop working, and they can't sign in to
-// MistWarp again until the ban is lifted. They see the reason, never who banned
-// them. The local list stays the source MistWarp checks on every request.
-
 def roturBanPath(string username) string (
   return "/bans/" ++ url.escape(roturUserRef(username))
 )
 
-// "yes" once Rotur has the ban, "refused" if Rotur won't ban them (they are on
-// the app's team, a classroom student, or not a Rotur account it can ban), ""
-// to try again later.
 def pushRoturBan(string username, string reason) string (
   string shown = reason.strip()
-  // Older bans from a report only name the report, which means nothing to the
-  // person banned.
-  if shown == "" or regex.match("^report r[A-Za-z0-9_-]+$", shown) (
+  boolean namesOnlyTheReport = regex.match("^report r[A-Za-z0-9_-]+$", shown)
+  if shown == "" or namesOnlyTheReport (
     shown = "You broke MistWarp's community guidelines."
   )
   object res = roturApp("PUT", roturBanPath(username), {reason: shown.trim(1, 300), note: "Banned in MistWarp"})
@@ -166,7 +154,6 @@ def pushRoturBan(string username, string reason) string (
   return ""
 )
 
-// Lifts the ban on Rotur. True once Rotur no longer has it.
 def liftRoturBan(string username) boolean (
   object res = roturApp("DELETE", roturBanPath(username), {})
   return res.ok or res.status == 404
@@ -205,8 +192,6 @@ def unbanUser(string username) boolean (
   if !cachedBans().bans.assertElse(object, {}).contains(key) (
     return false
   )
-  // Remembered until a sync has seen Rotur without it, so the sync keeps
-  // trying if Rotur couldn't be told, and never brings the ban back.
   boolean lifted = liftRoturBan(username)
   sync.lock("bans")
   defer sync.unlock("bans")
@@ -218,15 +203,11 @@ def unbanUser(string username) boolean (
   return saveBans(data)
 )
 
-// Keeps MistWarp's bans and the Rotur App's in step, every few minutes: bans
-// made or lifted on rotur.dev/me/developer reach MistWarp, and bans and unbans
-// Rotur couldn't be told about yet are sent again.
 def syncRoturBans() (
   number started = timestamp
   object snapshot = bansSnapshotLocked()
   object snapshotBans = snapshot.bans.assertElse(object, {})
   object snapshotUnbans = snapshot.unbans
-  // Unbans first, so the list read next already reflects them.
   object liftedNow = {}
   array unbanKeys = snapshotUnbans.getKeys()
   for i unbanKeys.len (
@@ -239,8 +220,6 @@ def syncRoturBans() (
   if !res.ok (
     return
   )
-  // Rotur lists accounts it has banned itself with no username, so match on
-  // the ID too.
   object remote = {}
   object remoteIds = {}
   array listed = (res.data.bans ?? []).assertElse(array, [])
@@ -277,7 +256,6 @@ def syncRoturBans() (
     object unban = unbans[key].assertElse(object, {})
     boolean lifted = unban.lifted == true or liftedNow.contains(key)
     if lifted and (unban.at ?? 0).toNum() < started (
-      // Rotur's list was read after it lifted this one.
       unbans.delete(key)
       changed = true
     ) else if lifted and unban.lifted != true (
@@ -286,8 +264,6 @@ def syncRoturBans() (
       changed = true
     )
   )
-  // Unbanned while the ban was being pushed: Rotur has it again, so lift it
-  // again next time.
   array pushedKeys = pushed.getKeys()
   for i pushedKeys.len (
     string key = pushedKeys[i].toStr()
@@ -307,21 +283,17 @@ def syncRoturBans() (
     )
     string state = (ban.rotur ?? "").toStr()
     if ban.by.toStr() == "rotur_sync" (
-      // Rotur enforces its own account bans now; these copies were never lifted.
       bans.delete(key)
       changed = true
     ) else if state == "" and pushed.contains(key) and pushed[key] != "" (
       ban.rotur = pushed[key]
       bans[key] = ban
       changed = true
     ) else if state == "yes" and ban.created.toNum() < started and !remote.contains(key) and !remoteIds.contains(userIdForUsername(key)) (
-      // Lifted on rotur.dev.
       bans.delete(key)
       changed = true
     )
   )
-  // Banned again while the old ban was being lifted: the lift may have
-  // removed the new ban from Rotur, so push it again.
   array liftedKeys = liftedNow.getKeys()
   for i liftedKeys.len (
     string key = liftedKeys[i].toStr()
@@ -821,8 +793,6 @@ def handleAdminSearchProjects(*serve.Context c) (
   c.json(200, {ok: true, projects: matches})
 )
 
-// Who a report is about, as worked out from the content when it was filed.
-// Older reports are worked out now, never from what the reporter said.
 def reportTargetUser(object found) string (
   string subject = (found.subject ?? "").toStr()
   if subject != "" (

src/admin/banned-owners.osl

@@ -2,11 +2,6 @@ export {
   entryOwnerBanned
 }
 
-// A banned user's projects are hidden everywhere except from admins. Bans are
-// kept by username; the set also holds each banned user's ID, so a project
-// still matches if its stored owner name is out of date after a rename.
-// Nothing is moved or deleted: lifting the ban brings everything back.
-
 def bannedOwnerSet(object bans, object idsByKey) object (
   object names = {}
   object ids = {}
@@ -38,13 +33,10 @@ def entryOwnerBanned(object banned, object entry) boolean (
   return ownerInBannedSet(banned, (entry.owner ?? "").toStr(), (entry.ownerId ?? "").toStr())
 )
 
-// A direct lookup of a hidden project answers like a missing one.
 def bannedProjectHidden(object banned, object project, boolean isAdmin) boolean (
   return !isAdmin and entryOwnerBanned(banned, project)
 )
 
-// Always a new list, so callers can sort it without touching a cached one.
-// Admins get every entry, with banned owners' copies marked ownerBanned.
 def projectEntriesVisible(array entries, object banned, boolean isAdmin) array (
   array visible = []
   for i entries.len (

src/admin/banned-owners.test.osl

@@ -22,7 +22,6 @@ array entries = [
   {id: "e", owner: "bob", ownerId: "u-bob"}
 ]
 
-// Bans are kept by normalized username; IDs are looked up when the set is built.
 object bans = {spammer: {reason: "spam"}, legacy: {reason: "spam"}}
 object banned = bannedOwnerSet(bans, {spammer: "u-spam", legacy: ""})
 
@@ -42,8 +41,6 @@ testing.assert(bannedProjectHidden(banned, entries[2], false), "a direct lookup
 testing.assert(!bannedProjectHidden(banned, entries[2], true), "a direct lookup by an admin works")
 testing.assert(!bannedProjectHidden(banned, entries[1], false), "other owners' projects are found")
 
-// Unbanning, or the Rotur sync dropping a ban once it ends there, rewrites the
-// bans file and the set is rebuilt from it.
 object afterUnban = bannedOwnerSet({legacy: {reason: "spam"}}, {legacy: ""})
 testing.assert(ids(projectEntriesVisible(entries, afterUnban, false)) == "abce", "an unbanned owner's projects come back")
 testing.assert(!bannedProjectHidden(afterUnban, entries[2], false), "an unbanned owner's project is found again")

src/admin/errors.test.osl

@@ -7,7 +7,7 @@ import "std:regex"
 import "../http/request-body.osl"
 
 string path = "/tmp/mistwarp-errors-" ++ timestamp.toStr() ++ ".db"
-auto testDb = db.open(path)
+*db.DB testDb = db.open(path)
 *db.Collection errorDocs = testDb.collection("site_errors")
 
 array errorReportOrigins = ["https://mistwarp.org"]

src/admin/project-moderation.osl

@@ -12,6 +12,7 @@ export {
   viewerSeesShadowBanned
 }
 
+// osl-disable-next-line OSL-UNTYPED-GLOBAL
 auto shadowBansCache = cache.new({capacity: 2, ttl: 300000})
 
 def loadShadowBans() object (

src/auth/account-controls.osl

@@ -190,10 +190,6 @@ def handleExportMyData(*serve.Context c) (
   })
 )
 
-// Delete everything MistWarp holds about someone: their projects and what
-// hangs off them, their community records, game data and backpack, then the
-// account itself. Used when they ask, and when Rotur says they deleted their
-// Rotur account, left MistWarp or were banned from Rotur (rotur-webhooks.osl).
 def eraseUser(string userId, string username) (
   eraseClassroomTeacher(userId)
   purgeGameDataForUser(userId)
@@ -248,7 +244,6 @@ def eraseUser(string userId, string username) (
   purgeUser(userId, username)
   removeUserFromSafetyLists(username)
   void fs.remove(safetyPath(username))
-  // Nothing of theirs is stored any more, so none of it counts.
   void ledgerFreeAll(userId)
 )
 

src/auth/agreement.osl

@@ -20,19 +20,17 @@ def loadAgreement() object (
   if metaResult.ok (
     meta = metaResult.data.assert(object)
   )
-  // Read text from .md file, fall back to the text field in json
   if fs.exists(agreementTextPath()) (
     meta.text = fs.readFile(agreementTextPath()).unwrapOr("")
   )
   return meta
 )
 
 def saveAgreement(object data) boolean (
-  // Write metadata to json, text to md
   string text = data.text
-  data.text = "" // don't store the text in json
+  data.text = ""
   bool ok = writeFile(agreementMetaPath(), data)
-  data.text = text // restore
+  data.text = text
   if text != "" (
     if !fs.writeFile(agreementTextPath(), text) (
       ok = false

src/auth/auth.osl

@@ -7,7 +7,7 @@ export {
   setAuthCookie
 }
 
-auto sessionCache = cache.new({capacity: 10000, ttl: 3600000})
+*cache.Cache sessionCache = cache.new({capacity: 10000, ttl: 3600000})
 
 def setAuthCookie(*serve.Context c, string sessionId) (
   c.setCookie("auth_token", sessionId, {maxAge: 604800000, path: "/", domain: "", secure: appURL.startsWith("https://"), httpOnly: true})
@@ -135,18 +135,13 @@ def handleRoturAuth(*serve.Context c) (
     return
   )
 
-  // The editor makes validators keyed to MistWarp's Rotur App, which its own
-  // sign-in token can do without validators:generate; Rotur then also refuses
-  // anyone the app has banned. Older editors use the old key. A validator
-  // checked against the wrong key is just invalid, so try the next.
   array keys = [roturAppKey]
   if roturClientId != "" (
     keys = [roturClientId, roturAppKey]
   )
   object data = {}
   for i keys.len (
     object resp = requests.get(roturBase ++ "/validate?key=" ++ url.escape(keys[i].toStr()) ++ "&v=" ++ url.escape(validator)).unwrapOr({body: null, error: "request failed", headers: {}, raw: null, status: 0, success: false})
-    // Rotur answers every validator with 200, valid or not.
     number status = (resp.status ?? 0).toNum().toInt()
     if status < 200 or status >= 300 (
       c.json(401, {ok: false, error: "failed to validate token"})
@@ -163,13 +158,10 @@ def handleRoturAuth(*serve.Context c) (
     )
   )
 
-  // Rotur answers every "no" with valid false and a code. Only a real "yes"
-  // signs anyone in.
   if data.valid != true (
     string code = (data.code ?? "").toStr()
     if code != "" and code != "invalid_validator" (
       object refusal = {ok: false, code: code, error: (data.error ?? "Rotur refused this sign-in.").toStr(), redirectUrl: (data.redirect_url ?? "https://rotur.dev/me").toStr()}
-      // An app ban says why, and until when.
       if (data.reason ?? "").toStr() != "" (
         refusal.reason = data.reason.toStr()
       )
@@ -183,8 +175,6 @@ def handleRoturAuth(*serve.Context c) (
     return
   )
 
-  // Old-key validators skip Rotur's checks for the app. One made by another
-  // Rotur App's sign-in was passed on to MistWarp, so it isn't accepted.
   string madeBy = (data.app ?? "").toStr()
   if roturClientId != "" and madeBy.startsWith("app_") and madeBy != roturClientId (
     c.json(401, {ok: false, error: "this sign-in was made for another app", redirectUrl: "https://rotur.dev"})