Skip to content

feat(platform): state every backend domain's rules in a spec that tests hold - #4420

Merged
larryro merged 59 commits into
mainfrom
feat/domain-specs-rollout
Oct 6, 2026
Merged

larryro merged 59 commits into
mainfrom
feat/domain-specs-rollout

Conversation

@larryro

@larryro larryro commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

#4351 introduced the domain spec format and trialled it on tasks. This gives each of the other 57 backend domains its own backend/domains/<domain>/spec.md in that format, and names every rule in the title of the test that holds it.

  • 57 new specs, 480 rules. Each spec covers part of its domain and lists what it leaves out under Not yet: the parts not covered, the rules the code keeps that no test holds yet, and the questions nobody has decided.
  • Titles only, almost everywhere. About 1,000 existing test and describe titles gain their rule's ID. No assertion of an existing test changes.
  • 87 new test titles (about 110 cases, some parameterised) new tests, where a rule was clearly intended (code and docs agree) and nothing held it. The larger additions: members/service.roles.test.ts (who can change a role or remove a member), two_factor/service.enforcement.test.ts (when a second factor is required), sandbox/sessions.admission.test.ts (the limit on sandboxes running at once), provisioning/service.seed.test.ts, changelog/*.test.ts, control/routes.test.ts. A number a spec states that tests only reached through a named constant is pinned by a one-line test.
  • One shared change. Every page under docs/en/platform/ is now an input of the platform test task (turbo.json, with its OUTSIDE_READS entry), so a spec can link its Docs page without a per-page registration. Of the last 150 commits that touched those pages, one did not also touch platform code, so the cache cost is small.
  • .agents/repo.md and backend/README.md no longer call the format a trial.

For the reviewer:

  • 17 questions are left undecided (18 with the one tasks already carried) under Not yet, each with both readings and where they come from. The ones that look like defects rather than open intent:
    • automations: who can stop a run differs between the app (any member / a project editor), the API and MCP (owner, admin or developer, plus edit access) and the task panel (the task's work gate); a read-only member can start a test run in a project from the app but not over the API; a schedule or event still starts a run in an archived project when the automation is installed there alone (contradicts TASK-R7); creating an automation inside an archived or unreadable project is not refused; the docs say a missed schedule occurrence is not replayed, the code replays the latest one within an hour; the docs say deploying reads the recorded test verdict, the code re-runs the tests.
    • knowledge: the credential scan reads a file's raw bytes, not its extracted text, so a key inside a .docx, .xlsx or compressed PDF is indexed (a test even holds that binary content is allowed); and when the PII scrubber cannot be built, documents are indexed unmasked with one logged error (tested, stated as the rule's exception).
    • conversations: an expired conversation leaves search and the attachment list but still shows in the Inbox and takes replies; the member role can be assigned a conversation it cannot reply to, close, or mark as read; the bulk send dialog says the action cannot be undone although each reply gets the 10-second undo window.
    • members: the docs say you cannot change your own role; the server has no such check.
    • login_attempts: a policy can be switched off, but the strictest-policy selection answers the enabled default when every organization's policy is off, so the lock always applies.
    • sandbox: the docs say a project agent's run started with an API key counts toward that key; the code books the key only for an automation's own agent step.
  • Whether a spec is mandatory for a new domain is not decided here. The guard still checks only the specs that exist. Requiring one per domain is a one-test change if that is wanted.
  • The specs were written from the tests, the code and the user docs. Whether each rule is the intended one is what this review is for.

Checklist

  • bun run check passes (format, lint, typecheck, all tests): run as its pieces on the rebased branch rather than the one command. oxfmt --check on every changed file, oxlint --type-aware on every changed test file, platform tsc --noEmit, the whole platform server test project (1,064 files, 15,188 tests, 0 failures), lint:links, lint:conflicts.
  • bun run lint:sast passes (Opengrep): 476 rules, 0 findings.
  • Translations: N/A, no user-visible string.
  • Docs: N/A for the user docs. The contributor docs (.agents/repo.md, backend/README.md, spec-template.md) are updated.
  • README.md / README.de.md / README.fr.md: N/A.

Test plan

  • bun run --filter @tale/platform test -- tests/guards/domain-specs.guard.test.ts tests/guards/turbo-inputs.guard.test.ts passes.
  • Remove a tag from any tagged title (for example [TEAM-R6] in backend/domains/teams/routes.test.ts) and run the guard: it fails with TEAM-R6 is held by no test.
  • Read a spec next to its domain's user docs page and its manual suite; a rule that contradicts either is a finding.

larryro added 30 commits October 6, 2026 09:22
@larryro
larryro merged commit 464e1d9 into main Oct 6, 2026
82 checks passed
@larryro
larryro deleted the feat/domain-specs-rollout branch October 6, 2026 02:13

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge review of #4420, slice 1 (CICD + WORKFLOW): accepted, with one non-blocking finding.

  • Commits: the merge on main is 464e1d9b7a1f15d854e622ec346fc446bd5fc65c (parent 75f60ead6), still main's head at review time; the PR head is 1948429b0e82316ca578e19b9dee6b0b6412bfe5.
  • Reviewer: TALE-361 Review B, agent #4 (5f5307c9), run 77b84ba5. Fleet coordination routed this slice.
  • Implementation: authored and merged by @larryro at 02:13:37Z, with no independent review.
  • Method: coordination-class reads only (the GitHub API, and git show/git diff of single files). I ran no tests, builds or installs, because the host's admission score was 4.89 (pressured).

Composition

  • 346 files:
    • 285 test files, all matching *.test.ts(x) or *.spec.ts (278 modified, 7 added);
    • 57 added backend/domains/*/spec.md;
    • 3 contributor docs: .agents/repo.md, backend/README.md and spec-template.md;
    • services/platform/turbo.json (+1).
  • No production source, config, schema or migration file changed. The tree has 58 domains and 58 specs.
  • This supports NOT_APPLICABLE for the runtime topics (security, data, performance, resources, deployment, recovery, providers, observability). The reviewer of record decides.

CICD: PASS

Question Finding
Which tests read docs/en/platform/**? Only tests/guards/domain-specs.guard.test.ts reads them; its OUTSIDE_READS entry was added in turbo-inputs.guard.test.ts. It checks that each spec's Docs link names an existing, hashed page.
Could the glob be narrower? Not usefully. All 67 files under it are .md, and the only narrower form is per-page registration, which the PR avoids by design.
Cache validity and CI minutes The Unit (platform) shards run on every push and PR, with no path filter, and reuse Turbo's local cache through the Actions cache (cache-scope: test-platform-N). Without the new input, a docs-only change that moves a linked page would hit that cache and skip the guard; with it, the guard runs again. The cost: a docs-only change re-runs both shards, about 7–9 runner minutes at today's 187–296 s per shard. The author's frequency claim checks out: of the last 150 commits touching docs/en/platform, one (a825a0f09) changed no platform code.
Do the guard and the input list agree? Yes. The input $TURBO_ROOT$/docs/en/platform/**, the OUTSIDE_READS entry docs/en/platform and the guard's assertions all match: an English page at any depth reads as hashed, a German page as unhashed, and the named tasks page counts in every locale.
Runtime of the new tests No measurable increase, with one run each. Unit (platform 1/2) took 224 s at the PR head, 187 s at the merge and 215 s at its parent; Unit (platform 2/2) took 296, 251 and 271 s. The merge's Checks run (37403096562) and the PR head's (37399577778) passed. The parent's own Checks run failed on Unit (workspaces).

WORKFLOW: PASS, with one non-blocking finding

  • .agents/repo.md:
    • "Every domain under backend/domains/ has a spec (2026-10)" is true at 464e1d9b (58 of 58).
    • The crawler ceiling change from 25 MB to 100 MiB matches the code: DEFAULT_CRAWL_DOCUMENT_MAX_BYTES = 100 * 1024 * 1024 in backend/core/knowledge/crawl_limits.ts. That value has held since #3999, and operators can set it with KNOWLEDGE_CRAWL_DOCUMENT_MAX_BYTES. docs/en/platform/knowledge/crawling.md states the same, so the old 25 MB was stale.
  • domain-specs.guard.test.ts (blob cefb4f53fa):
    • Its verdict is deterministic; readdir order only changes the order of failure messages.
    • Its failure modes are clear, reported at file and line: a rule no test holds; a title naming a rule no spec states; a duplicate prefix; a Docs link that's missing or unhashed; a grammar error.
    • A canary keeps the scan from passing empty.
    • It scans every test file under services/platform except build and output directories. So an untracked local test file can fail it on a developer machine, though not in CI.
  • Finding W-1 (low, non-blocking): the agent contract now states "Every domain … has a spec" as a fact that no guard holds. The PR body says requiring one per domain "is not decided here" and would be "a one-test change". So the next new domain will make the contract false while CI stays green. Suggestion: add the one-test requirement, or word the line as a dated fact and name the open decision.
  • Open PRs (89: 27 CLEAN, 27 UNSTABLE, 35 DIRTY):
    • 5 of the 35 DIRTY PRs share files with #4420: #4270 (7 files), #4238 (6), and #4263, #4241 and #4220 (1 each).
    • #4409 is DIRTY but shares none of its 7 files with #4420, so its conflict comes from other changes on main.
    • Not checked: whether CLEAN or UNSTABLE PRs that rename or delete a tagged test, or move a linked docs page, now fail the guard after a rebase. That needs a test run on each composition.

Left NOT_RUN for a distinct reviewer, as routed

  • CODE:
    • the quality of the 87 new test titles (about 110 cases);
    • proof that the roughly 1,000 renamed titles changed no assertion. The modified test files' −1,123 lines fit title-only edits, but I didn't verify that.
  • DOCS: the 57 specs and their 17 open questions.

Limits

  • No local runs; the runtime comparison is one run per commit.
  • A GraphQL listing of open PRs with their file lists returned HTTP 502. So I read the overlap over REST, for the 35 DIRTY PRs only.

@yannickmonney

Copy link
Copy Markdown
Contributor

Post-merge CODE evidence: test-title renames (TALE-359 run 9667927a, agent #2; I'm not the author)

Scope: the 278 test files modified by squash 464e1d9b7 (parent 75f60ead6), compared before and after with read-only git show. This is not a review of the 87 new tests, which stay with the CODE/DOCS owner (#6).

Method: remove the rule-ID title tags ( [XXX-R1], [XXX-R1, XXX-R2]) from the new file and compare it with the old one. Where they still differ, check that every old assertion line still exists in the new file, ignoring whitespace (as a multiset).

Results:

  • 241 files: byte-identical once the tags are removed, so only titles changed.
  • 22 files: additions only. Their old lines are unchanged and new tests were added.
  • 13 files: blocks moved or re-wrapped, but every old assertion line is still present: crawl_action.redirects, embedding_credentials, store.begin-run, store.delete-run, contacts/routes, api-sync.teardown, image_proxy/service, knowledge/service.provider-refusal, products/images, webdav/routes, v1-browser-sessions, v1-threads.scope and turbo-inputs.guard.
  • v1-automations.project-scope.test.ts: one old one-line assertion now appears re-wrapped by the formatter inside a new block (lines 262–264), and the assertion is still there. It's re-wrapped, not removed.
  • tests/guards/domain-specs.guard.test.ts: 8 old assertion lines are gone. This is the guard for the new spec and tag system, and its probe cases (named(...), tree.outsideInput(...)) were rewritten on purpose. The CODE owner should review these semantics.

Conclusion: the renames changed no assertion except in the deliberately rewritten spec guard. Evidence, with per-file classes and the multiset check, is in TALE-359's delivery box (TALE-359-evidence-20261006-0645.tar.gz).

@yannickmonney yannickmonney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge review of #4420, slices 2–3 (CODE and DOCS). The guard rewrite passes. The new tests and the specs fail, with one HIGH finding. All findings are tracked in #4424, #4425 and #4426.

  • Commits: the squash on main is 464e1d9b7a1f15d854e622ec346fc446bd5fc65c (parent 75f60ead6); the PR head is 1948429b0e82316ca578e19b9dee6b0b6412bfe5.
    • Every file cited below has the same blob at 464e1d9b and 1948429b, so the line numbers hold for both. The files are also unchanged on main 8b750caf30.
  • Reviewer: TALE-947, agent #6 (53fe2a33), run 2d1106e9. Fleet coordination routed this (run 52f43052). I am not the author and wrote nothing in #4420.
  • Method: coordination-class reads only:
    • the GitHub API: PR files with their patches, and one code search, for two_factor_grace;
    • git show and git diff of single files.
  • Not run: tests, mutations, builds and installs; none of these were admitted. Each "stays green" below is reasoned from the code.
  • Sub-reviews: six read-only sub-reviews split the test packages. Before posting, I re-read every HIGH and MEDIUM finding at its cited lines. I read the legal_holds, teams, agent_secrets, erasure and scim specs against the code myself.
  • Reused, not redone: slice 1 by #4 (review 5423763658: CICD PASS, WORKFLOW PASS plus W-1) and #2's rename evidence (comment 6009574548).

Dispositions

Slice Disposition
CODE: the guard rewrite (tests/guards/domain-specs.guard.test.ts, blob cefb4f53fa) PASS. Two assertion lines were removed, not eight. Both are replaced by checks that are equal or stronger.
CODE: the new tests (99 candidate titles in 37 files; the PR counts 87) FAIL, post-merge and tracked. Most are meaningful and deterministic. One asserts a security property the code does not have (H1, #4424). Eight more name a rule, or part of one, that their assertions do not hold (#4426).
DOCS: rules against the code (21 of 57 specs, about 90 of 480 rules) FAIL, tracked. Ten statements in eight specs disagree with the code: H1 in #4424 and nine in #4425. One Not yet section also omits two known gaps (#4425, row 10).
DOCS: whether Not yet is honest (same 21 specs) PASS, with one exception. The conversations spec leaves out two gaps the PR body says it lists (#4425, row 10). Every other PR-body claim I checked is disclosed.
DOCS: links PASS. All 45 distinct relative link targets in the 57 specs resolve at 464e1d9b. The 32 Docs pages are all under docs/en/platform/. There are no #fragment links and no external links. 19 specs carry no Docs link, which the template allows.

1. CODE: the guard rewrite

The file's diff is +8/−2 (git diff 75000ddf5 cefb4f53f, one hunk at :669). #2's comment counted 8 removed lines. That count includes six probe assertions that contain [PROBE-R1] or [OTHER-R1]: stripping title tags from the new file before the multiset comparison makes them look removed. They are byte-identical, only shifted down six lines.

Old line (75000ddf5) Old assertion Its replacement (cefb4f53f) Verdict
:672 outsideInput('docs/en/platform/projects/tasks.md') → 'hashed' :673: docs/de/platform/projects/tasks.md → 'hashed' (the page listed by name, in every locale). :677 and :678: docs/en/platform/models.md and docs/en/platform/projects/overview.md → 'hashed' (the new docs/en/platform/** glob at depths 1 and 2). Stronger. Removing either the named input or the new glob from turbo.json now fails the test. Before, only the named input was pinned. The English tasks page matches both inputs.
:675 outsideInput('docs/en/platform/projects/overview.md') .not.toBe('hashed') Inverted on purpose, because every English page is now an input. The negative case it carried moved to :681: docs/de/platform/projects/overview.md → toBe('unhashed'). Stronger. It pins the exact state. The old .not.toBe('hashed') would also have passed on 'missing', a deleted page.
:688, :694, :697, :700, :709, :719 The named(…) probes (run, it.each, test.describe, multi-line template, it.todo, other prefix) :694, :700, :703, :706, :715, :725 Unchanged (byte-identical).

outsideInput reads the real tree and the real turbo.json inputs (:474-479), so these checks hold the shipped configuration, not a fixture.

2. CODE: the new tests

Coverage. I enumerated new titles from the PR patches: added test-call lines with no matching removed title, tags ignored.

  • The 7 new files, 42 titles, read in full: members/service.roles.test.ts (8), two_factor/service.enforcement.test.ts (8), sandbox/sessions.admission.test.ts (10), provisioning/service.seed.test.ts (5), changelog/routes.test.ts (3), changelog/service.test.ts (5), control/routes.test.ts (3).
  • The new blocks in 30 modified files, 57 titles, also all read: conversations 20, knowledge 15, automations 8, sandbox 7, and chat, contacts, products, image_proxy, users and browser sessions 7.

What holds:

  • Meaningful assertions. Most new refusal tests pin the production guard: deleting the guard line fails the test. Most also assert that nothing was written, called or reserved. Examples:
    • the MEMBER-R1–R5 refusals (members/service.ts:288-303, :377-441);
    • the SBX-R8/R9 admission and resume checks (sandbox/sessions.ts:174-205, :428-441);
    • the CTRL-R1 404 (control/routes.ts:42-45);
    • the CLOG-R2 three-page cap (changelog/service.ts:21);
    • the CONV-R11 file limits, tested on both sides of each edge (core/conversations/attachments.test.ts:28);
    • the KNOW-R10 refusal before any credential read (core/knowledge/embedding.ts:851);
    • the BSESS-R5 lifetime, including the computed expiry.
  • Constant pins. About ten one-line tests pin a named constant, which the PR states as intended: 5 failures, 24 h and 2 min, 1,000 rows (two of these), 10 MB, 100 characters, 200 conversations, the default limit of 2, 30 days, and the status codes. In each case the pinned constant is the one enforcement uses. Exceptions are rows 7 and 8 of #4426.
  • Deterministic. I found no wall-clock or network dependence:
    • two_factor uses fake timers with setSystemTime;
    • browser sessions spies on Date.now and restores it;
    • env stubs are removed in afterEach;
    • changelog injects its fetcher, which bypasses the page cache.
    • One ambient-env dependency remains (LOW, not filed): the private-host rows at core/knowledge/crawl_action.redirects.test.ts:235 fail in a shell that exports TALE_ALLOW_PRIVATE_CRAWL_HOSTS=1, because privateCrawlHostsAllowed reads process.env at call time (lib/net/crawl-host-policy.ts:32).
  • Titles against rules. Every tag names a rule whose text covers the test's subject, except for the over-claims below.

Findings (CODE). Paths are relative to services/platform/backend/.

  • H1 (HIGH), #4424: domains/two_factor/service.enforcement.test.ts:154-169, "…applies a shorter one at once [TFA-R4]".
    • It asserts the shortened deadline at one frozen instant.
    • The code recomputes cap = now + days on every check (domains/two_factor/service.ts:268, :276-277), so a shortened policy never blocks anyone before the original anchor. The displayed deadline slides forward each day.
    • The spec says the opposite, and so does the code comment at :274-275.
  • M1 (MEDIUM), #4426 row 1: domains/control/routes.test.ts:42, the "another token" row.
    • The wrong token's length differs from the configured one (13 against 23 characters), so a.length === b.length && timingSafeEqual(a, b) (domains/control/routes.ts:35) short-circuits.
    • Mutation: return a.length === b.length; keeps all five cases green.
  • M2 (MEDIUM), #4426 row 2: domains/automations/store.delete-run.test.ts:199-200, :216.
    • The fake answers any SELECT status …, so the queued/running/waiting set (store.ts:2865) is not held.
    • Dropping 'waiting' stays green, though AUTO-R15's own example is a waiting run.
  • M3 (MEDIUM), #4426 row 3: domains/conversations/send.test.ts:105-117, :884.
    • The fake answers any FROM app.approvals, so which draft is completed (draft.ts:194-197) is not held.
  • M4 (MEDIUM), #4426 row 4: domains/two_factor/service.enforcement.test.ts:107-113.
    • The strict organization is listed last, so a merge where the last organization wins passes.
  • L1–L4 (LOW), #4426 rows 5–8:
    • the sandbox ?orgId= checks are true by construction under the middleware mock (domains/sandbox/routes.test.ts:62-67);
    • "takes it out of the index" does not assert the chunk purge (core/knowledge/crawl_action.ts:1514);
    • domains/sandbox/workspace-cleanup.test.ts:81 is a constant snapshot titled as the fallback;
    • one spelling-only test carries five KNOW tags.
  • Not filed, from the sub-reviews (LOW; fixtures and wording):
    • the MEMBER-R4 fixture is a state the database cannot hold: a second admin with an admin count of 1;
    • the self-removal fixture's role does not match;
    • CLOG-R2's version boundary is not exercised;
    • writes === [] in the PROVN-R2 test cannot see the presentation-update path.
    • I re-read the first; the others are recorded in the TALE-947 delivery box.

3. DOCS: the 57 specs

Sample: 21 specs, about 91 rules, read against production code rather than tests:

Spec Rules read against code
members R1–R7, R9
two_factor R1–R5
sandbox R1–R3, R5, R6, R8, R9, R12
provisioning R1–R6
changelog R1–R4
control R1–R4
login_attempts R1–R4
conversations R1, R3, R4, R7, R9, R11, R12, R14–R16
knowledge R3, R8–R15
automations R1, R9, R13, R15
teams R2–R7, R10
legal_holds R1–R3
agent_secrets R1–R4
erasure R3, R4, R8
scim R1–R3, R5, R8
chat, contacts, products, image_proxy, users, browser_sessions the rule each new test names

The other 36 specs were checked for links only. Their shape is held by the guard.

Contradicted by the code, or missing from Not yet:

  • H1, two_factor TFA-R4 (domains/two_factor/spec.md:41-44): "A policy that is made shorter applies at once". Tracked in #4424.
  • Ten rows in #4425, of which five are MEDIUM:
    • erasure ERASE-R4 and its Not yet: a request the watchdog failed is NOT_RETRIABLE (domains/erasure/service.ts:572-580).
    • changelog CLOG-R4: a 200 page that parses to nothing answers 200 { releases: [] }.
    • knowledge KNOW-R9: "one exception" is in fact two fail-open paths (core/knowledge/pii_gate.ts:165-166, :189-196), and both apply in Block mode.
    • sandbox SBX-R3: "an organization named in the request is ignored" is false; auth/org.ts:48, :76 use it.
    • two_factor TFA-R1: organizations that do not enforce still shape the merged grace and exemption (core/governance/helpers.ts:17-19).
  • The five LOW rows:
    • TEAM-R3's error code for add and remove;
    • PROVN-R4: an unreadable catalog returns the empty result;
    • SBX-R8: a stopped workspace that is pinned resumes without the limit check;
    • HOLD-R3: self-approval skips the wait;
    • two conversations gaps that Not yet leaves out.

Not yet honesty. Every PR-body claim I checked is disclosed, at these lines:

  • automations: all six open questions (automations/spec.md:287-332). I re-read one against the code: the read-only test run, routes.ts:807-828.
  • knowledge: the raw-bytes credential scan (:308-313).
  • members: the self role change (:89-93). The docs say it is refused (members-and-roles.md:63), and updateMemberRole has no such check.
  • login_attempts: a disabled policy is never reached (:69-75).
  • sandbox: the API-key booking (:265-270).
  • conversations: the expired conversation (:343-351).
  • Exceptions: knowledge's fail-open is written into the rule rather than Not yet (#4425 row 3), and conversations is missing two items (#4425 row 10).

Unfiled lead, MEDIUM, plausible.

  • The gap: auth/auth.ts:775-780 disables the plugin's /organization/leave and its team-member doors, but not /organization/update-member-role or /organization/remove-member. The comment at :764-768 says the open remove-member door runs only the afterRemoveMember cascade. So the app's creator, last-admin and legal-hold guards and its audit row (MEMBER-R3–R5) would not run on those doors, and the members spec does not mention them.
  • Not verified: the library's own checks, and whether these routes are reachable. Confirming it needs a library read or a check probe.

Limits

  • Nothing executed. No tests, mutations, builds or installs were run, because they were not admitted.
  • Sample size. 21 of the 57 specs were read against code, about 19 % of the rules. The other 36 had links only.
  • No broad scans. Callers outside the files I read were not searched; the only exception is one GitHub code search for two_factor_grace.
  • Who verified what. I re-verified every HIGH and MEDIUM finding. The LOW notes from sub-reviews are labelled as such above.

Requested next, not run here. A named check phase to:

  • confirm H1 with a probe that advances fake time past a shortened deadline (expected blocked; red at 464e1d9b);
  • confirm the #4426 mutations.

Root's rule applies: a blocking finding overrides ACCEPT until it is independently closed at the exact head, with proof. I did no merge, push, CI action or card move.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants