Repository navigation
feat(platform): state every backend domain's rules in a spec that tests hold - #4420
Conversation
yannickmonney
left a comment
There was a problem hiding this comment.
Post-merge review of #4420, slice 1 (CICD + WORKFLOW): accepted, with one non-blocking finding.
- Commits: the merge on main is
464e1d9b7a1f15d854e622ec346fc446bd5fc65c(parent75f60ead6), still main's head at review time; the PR head is1948429b0e82316ca578e19b9dee6b0b6412bfe5. - Reviewer: TALE-361 Review B, agent #4 (
5f5307c9), run77b84ba5. 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 diffof 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.mdandspec-template.md; services/platform/turbo.json(+1).
- 285 test files, all matching
- 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 at464e1d9b(58 of 58). - The crawler ceiling change from 25 MB to 100 MiB matches the code:
DEFAULT_CRAWL_DOCUMENT_MAX_BYTES = 100 * 1024 * 1024inbackend/core/knowledge/crawl_limits.ts. That value has held since #3999, and operators can set it withKNOWLEDGE_CRAWL_DOCUMENT_MAX_BYTES.docs/en/platform/knowledge/crawling.mdstates the same, so the old 25 MB was stale.
- "Every domain under
domain-specs.guard.test.ts(blobcefb4f53fa):- Its verdict is deterministic;
readdirorder 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/platformexcept build and output directories. So an untracked local test file can fail it on a developer machine, though not in CI.
- Its verdict is deterministic;
- 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.
Post-merge CODE evidence: test-title renames (TALE-359 run
|
yannickmonney
left a comment
There was a problem hiding this comment.
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(parent75f60ead6); the PR head is1948429b0e82316ca578e19b9dee6b0b6412bfe5.- Every file cited below has the same blob at
464e1d9band1948429b, so the line numbers hold for both. The files are also unchanged on main8b750caf30.
- Every file cited below has the same blob at
- Reviewer: TALE-947, agent #6 (
53fe2a33), run2d1106e9. Fleet coordination routed this (run52f43052). 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 showandgit diffof single files.
- the GitHub API: PR files with their patches, and one code search, for
- 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.
- the MEMBER-R1–R5 refusals (
- 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_factoruses fake timers withsetSystemTime;- browser sessions spies on
Date.nowand 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:235fail in a shell that exportsTALE_ALLOW_PRIVATE_CRAWL_HOSTS=1, becauseprivateCrawlHostsAllowedreadsprocess.envat 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 + dayson 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.
- The wrong token's length differs from the configured one (13 against 23 characters), so
- M2 (MEDIUM), #4426 row 2:
domains/automations/store.delete-run.test.ts:199-200,:216.- The fake answers any
SELECT status …, so thequeued/running/waitingset (store.ts:2865) is not held. - Dropping
'waiting'stays green, though AUTO-R15's own example is a waiting run.
- The fake answers any
- 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.
- The fake answers any
- 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:81is a constant snapshot titled as the fallback;- one spelling-only test carries five KNOW tags.
- the sandbox
- 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,:76use it. - two_factor TFA-R1: organizations that do not enforce still shape the merged grace and exemption (
core/governance/helpers.ts:17-19).
- erasure ERASE-R4 and its Not yet: a request the watchdog failed is
- 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), andupdateMemberRolehas 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-780disables the plugin's/organization/leaveand its team-member doors, but not/organization/update-member-roleor/organization/remove-member. The comment at:764-768says the open remove-member door runs only theafterRemoveMembercascade. 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
checkprobe.
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 at464e1d9b); - 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.
Summary
#4351 introduced the domain spec format and trialled it on
tasks. This gives each of the other 57 backend domains its ownbackend/domains/<domain>/spec.mdin that format, and names every rule in the title of the test that holds it.describetitles gain their rule's ID. No assertion of an existing test changes.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.docs/en/platform/is now an input of the platformtesttask (turbo.json, with itsOUTSIDE_READSentry), 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.mdandbackend/README.mdno longer call the format a trial.For the reviewer:
tasksalready 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 (contradictsTASK-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,.xlsxor 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; thememberrole 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.Checklist
bun run checkpasses (format, lint, typecheck, all tests): run as its pieces on the rebased branch rather than the one command.oxfmt --checkon every changed file,oxlint --type-awareon every changed test file, platformtsc --noEmit, the whole platformservertest project (1,064 files, 15,188 tests, 0 failures),lint:links,lint:conflicts.bun run lint:sastpasses (Opengrep): 476 rules, 0 findings..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.tspasses.[TEAM-R6]inbackend/domains/teams/routes.test.ts) and run the guard: it fails withTEAM-R6 is held by no test.