Repository navigation
Commit ef256e6
Fixes #18785
Clause-②: no
One rule — "does this effective object permission grant this verb?" —
had three implementations. It now has one definition in
`@objectstack/spec` (`objectPermissionGrants`) and two consumers that
**ask** it: the enforcement door
`PermissionEvaluator.checkObjectPermission`, and the access-matrix
snapshot `buildAccessMatrix`.
## ⛔ The differential ran FIRST — nothing was converged before it
The card's own "Not measured" section named this as the cheap thing to
do first, and it forked the rest of the work. It was run against the
three implementations **as sources**, before any edit.
**Input enumeration, derived rather than hand-listed.** The bit set is
read out of the spec's own `EffectiveObjectPermissionSchema.shape` —
every key whose unwrapped inner type is `boolean` — which yields exactly
the eight declared bits: `allowCreate`, `allowDelete`, `allowEdit`,
`allowExport`, `allowRead`, `allowTransfer`, `modifyAllRecords`,
`viewAllRecords`. The verb targets come from the spec's own
`OBJECT_PERMISSION_VERBS` value range (six). Each bit takes **three**
states — `true`, `false`, absent — because every implementation compares
with `=== true` and absence is the state an author reaches by omission.
That is 3^8 = **6561 inputs**, the full cartesian product over the
declared bits, not a sample.
```
bits (derived) : allowCreate, allowDelete, allowEdit, allowExport,
allowRead, allowTransfer, modifyAllRecords, viewAllRecords [8]
bit states : true | false | absent [3]
verb targets : allowCreate, allowDelete, allowEdit, allowExport,
allowRead, allowTransfer [6]
inputs : 3^8 = 6561
cells spec-vs-evaluator : 39366
cells spec-vs-matrix : 26244 (buildAccessMatrix models 4 of 6 verbs)
absent-entry leg : 6 targets, all false = true
DISAGREEMENTS spec-vs-evaluator : 0
DISAGREEMENTS spec-vs-matrix : 0
VERDICT differential-disagreements=0
```
⇒ **All three agreed on every input.** This is therefore a pure
structural convergence and **no behaviour changes** — which is also why
no ruling was needed. Had any cell disagreed, the card said stop, and it
would have stopped.
**The control — the instrument can tell the two cases apart.** "They all
returned the same thing" proves nothing unless the harness can *see* a
disagreement, so the same run was repeated twice with exactly ONE cell
of the spec fold perturbed, once in each direction:
| control mutation | what it changes | disagreements found |
|:--|:--|:--|
| `create-bypass` | "Modify All Data manufactures create" (spec says
**true** where the consumers say false) | **2916** (1458 evaluator +
1458 matrix) |
| `read-narrow` | read stops bypassing on `modifyAllRecords` (spec says
**false** where the consumers say true) | **1944** (972 evaluator + 972
matrix) |
Both directions, both consumers, non-zero. Plus a second exhaustiveness
leg the enumeration cannot express: `objectPermissionGrants(undefined,
verb)` is `false` for all six targets, never a throw.
**The evaluator's operation map is checked, not assumed.**
`checkObjectPermission` is keyed on ObjectQL operations, not on `allow*`
bits, and its `OPERATION_TO_PERMISSION` is module-private. The harness's
inverse map is asserted against the exported `crudBucketForOperation`
before a single cell is compared, so a drifted mapping aborts the run
instead of quietly measuring the wrong pairs.
**The fold as the card records it, verified against each
implementation** rather than trusted: read bypasses on `viewAllRecords
|| modifyAllRecords`; write bypasses on `modifyAllRecords` alone and
never create; export is `grant ∧ read`. All three held, on all three
sides.
## What changed
**`packages/plugins/plugin-security/src/permission-evaluator.ts`** — the
per-set loop's three inline checks (the modify-all write bypass, the
read bypass, the bare bit) collapse to one
`objectPermissionGrants(objPerm, permKey)` call.
`OPERATION_TO_PERMISSION` is retyped `Record(string,
ObjectPermissionVerbTarget)`, which is what makes that call type-safe
and forces a future operation's bit to be a verb the spec actually
models.
**`packages/lint/src/build-access-matrix.ts`** — the four CRUD columns
become `objectPermissionGrants(entry, verb)`. The two super-user columns
stay **raw bits** on purpose: they report what the set *declares*, which
is the context a reviewer reads the CRUD columns against, and folding
them would destroy that.
**Two things deliberately NOT collapsed**, both documented at the site:
- **The export door keeps its cross-set shape.**
`checkObjectPermission('export', …)` asks `(∃ set granting export) ∧ (∃
set granting read)` across the whole resolved set list — the same answer
the `/me/permissions` most-permissive per-object merge hands the client,
and the same value the spec cell returns once that merge has happened.
Folding it per set would have **narrowed** the door to `∃ set (export ∧
read)`, denying a caller whose read and export grants arrive from two
different sets. A narrowing at the enforcement door is exactly what this
card forbids. A pin was added for it.
- **`MODIFY_ALL_WRITE_KEYS` is kept and exported, as an assertion rather
than a decision.** It no longer decides anything at runtime; it still
states, derived from this file's own dispatch vocabulary, *which* bits
the write-bypass class contains, and the new test holds the spec's fold
to it. Deleting it would have thrown away the #1883 derivation (a future
destructive op added to the map+set must make the spec go red, not
silently lose its bypass) and dangled the prose in four other files that
name it.
`packages/lint/src/index.ts` was **not touched** — `buildAccessMatrix`
was already exported, and the change is an import, not an export edit.
## Following #17469, and where this departs
#17469's landing is the model: `driver-sql`'s raw `field.multiple` reads
became one call onto the spec's `isMultiValueField`, behind a small
documented adapter (`isMultiValuedColumn`) whose docblock names the
ruling and says in one line that there is ONE definition.
**Followed** — one definition in the spec, consumers ask it, and each
call site carries a docblock that says what restating it would cost
rather than just what it does.
**Departed, three ways, each on purpose:**
1. **No local adapter.** #17469 needed one because its callers had
already applied their own `type` resolution and two spellings of that
default is how its drift started. These consumers hold the effective
entry itself, so the call is direct and an adapter would be a third
spelling with nothing to reconcile.
2. **No behaviour change and no ruling.** #17469 converged a **live
divergence**, so a maintainer had to say which predicate was
authoritative. Here the differential above proved agreement before
anything moved, so there was nothing to rule on — and per the card, if
it had disagreed this PR would not exist.
3. **No authoring-entrance half.** #17469 also tightened `FieldSchema`
to refuse the flag it had been accepting. Nothing here needed a spec
edit, and the dispatch is explicit that one would be a different lane.
## Ablation — each consumer proved to be ASKING, not agreeing by
coincidence
The pins were written **independently of the spec helper**, from the
rule itself. Asserting `consumer === objectPermissionGrants(...)` would
have been a tautology: both sides move together and the pin survives any
change to the fold, including a wrong one. Because they restate the rule
instead, one changed cell in the spec reddens them.
One `scripts/ablation-replace.mjs` process: mutate → rebuild spec →
prove the mutation reached `dist/` → run each consumer → restore, with
the restore proven on disk.
```
ablation-replace: anchor "case 'allowEdit': return permission.allowEdit === true || modifyAll;" x1 -> x0
ablation-replace: blob 0e6d690 -> ba220b986417
ablation-dist-preflight: marker present in 2 built files -- the ablation is live in the artifact the suite consumes
hit packages/spec/dist/security/index.js
hit packages/spec/dist/security/index.mjs
LINT-CONSUMER exit=1 Test Files 1 failed (1) Tests 2 failed | 6 passed (8)
SECURITY-CONSUMER exit=1 Test Files 1 failed (1) Tests 3 failed | 2 passed (5)
ablation-replace: ok restored: blob == HEAD (0e6d690) and `git diff HEAD` is empty
ablation-dist-preflight --absent: marker absent from all 216 built files
ablation-dist-preflight --absent: tree clean against HEAD -- nothing of this ablation is recorded outside dist/
git status --porcelain (whole tree) : empty
post-restore rerun : lint 8 passed (8) · plugin-security 5 passed (5)
```
The ablation cell was `allowEdit`'s write bypass — one cell, and
**both** consumers went red through it. Named failures:
- lint: `named cells: the fold as the rule states it`, `exhaustively
matches the fold over every declared bit combination`
- plugin-security: the same two, plus `the Modify-All write-bypass CLASS
is exactly what this file derives from its own dispatch map`
⚠️ The restore leg was rebuilt as well as reverted: a marker left in
`dist/` keeps mutated code live for every later run in the worktree, and
`--absent` is what proves it is gone.
## The enforcement door is security-relevant — every covering test file,
before and after
`checkObjectPermission` is exercised by exactly five test files, all in
`@objectstack/plugin-security`. Three other files name it **in prose
only** and call nothing —
`packages/spec/src/security/permission.test.ts`,
`packages/formula/src/permission-predicate.test.ts`,
`packages/runtime/src/domains/share-links-enforcement-context.test.ts` —
so they are listed for completeness, not counted.
| test file(s) | before | after |
|:--|--:|--:|
| the four pre-existing door files, run as one vitest invocation —
`admin-export-wildcard.test.ts`, `export-permission-axis.test.ts`,
`member-default-explicit-allow.test.ts`, `security-plugin.test.ts` |
**334 passed** | **334 passed** |
| `plugin-security/src/permission-evaluator.test.ts` — NEW, the file the
door never had | — | **5 passed** |
| **door total** | **334** | **339** |
| `lint/src/build-access-matrix.test.ts` (the matrix consumer) | **5
passed** | **8 passed** |
"Before" is a measurement, not an inference: the two source files were
reverted to the merge base and the new test file removed, the four door
files were run against that tree, and the revert was undone with `git
checkout HEAD --` and proved byte-exact (`git status --porcelain` empty,
`git diff HEAD` empty, and each of the four paths' `git hash-object`
equal to its blob at `HEAD`). **Nothing was removed or weakened** —
every pre-existing case is still there and still green.
## Verification
Measured at `a5e7c92e8`, base `4045781fa`.
| what | result |
|:--|:--|
| `pnpm --filter @objectstack/lint run test` | **4022 passed (4022)**,
106 files |
| `pnpm --filter @objectstack/plugin-security run test` | **2249 passed
(2249)**, 117 files |
| `pnpm --filter @objectstack/lint run typecheck` | exit **0** (incl.
test layer: 2 files / 6 errors / 2 pinned signatures held, unchanged) |
| `pnpm --filter @objectstack/plugin-security run typecheck` | exit
**0** (incl. test layer: 0 / 0 / 0) |
| `pnpm lint` (repo-wide eslint, the union — not a narrowing) | exit
**0** |
**Gates.** Derived with `node scripts/pm/dispatch-gates.mjs --repo
objectstack-ai/objectstack --commands`, every exit code captured
**before any pipe**, reconciled with `--ran`:
```
Run reconciliation — 63 derived, 63 run, 0 NOT-MEASURED, 0 UNRUN.
✓ dispatch-gates --ran: 63 derived famil(ies) accounted for — 63 run,
0 NOT-MEASURED (a DERIVED zero — all 63 recorded an exit code and none of them is 3).
```
Three of them first answered **exit 3 — PREREQUISITE NOT MET**, which is
neither a pass nor a fail. All three named the same prerequisite — no
built output on disk — and it was **discharged**, not reported around:
| gate | prerequisite it named | after `turbo run build` over all
packages |
|:--|:--|:--|
| `pnpm check:dual-build-cjs-loads` | 53 packages have no `dist/` | exit
**0** |
| `pnpm check:i18n` | the CLI plus the build closure of 9 extract-config
packages | exit **0** |
| `pnpm check:type-check-debt` | 14 workspace deps of ledgered packages
have no built type entry point | exit **0** |
The derivation was re-run after a fresh `git fetch origin main` (which
moved `origin/main` 4 commits): the family list came back
**byte-identical**, so nothing new was owed.
## Changeset — measured, not assumed
`skip-changeset` does not apply. Both packages publish, and the changed
bytes reach `dist`:
| | `private` | `files[]` | changed bytes reach `dist`? |
|:--|:--|:--|:--|
| `@objectstack/lint` | unset (public) | `dist`, `README.md`,
`CHANGELOG.md` | yes — `src/build-access-matrix.ts` is built into
`dist/index.js` / `.cjs` |
| `@objectstack/plugin-security` | unset (public) | `dist`, `README.md`,
`CHANGELOG.md` | yes — `src/permission-evaluator.ts` likewise |
⇒ `.changeset/18785-one-permission-fold.md`, **`patch` for both**. Not
breaking: no export is removed or renamed, no authorable key changes
meaning, and the differential is the evidence that no answer moves.
`@objectstack/spec/security`'s `./security` subpath carries both an
`import` and a `require` condition, so the new value import is safe on
the CJS side of both dual builds — `check:dual-build-cjs-loads` confirms
it against real emitted bytes.
## Acceptance notes
⛔ No cards filed from here — the seat files everything.
**① A fourth site reads the same bits, and its own docblock claims to
mirror this door — measured, they differ on one bit.** The card's second
"Not measured" bullet asked whether any consumer outside these three
folds the same bits a fourth time. It does.
`packages/lint/src/validate-security-posture.ts` has
`grantsObjectAccess`, whose docblock says verbatim:
> Any of the four CRUD bits, or a super-user bypass (View/Modify All
Data), counts — this mirrors the runtime `checkObjectPermission` gate
(ADR-0066 D2): that gate returns true if ANY set contributes one of
these for the object.
Its disjunction omits `allowTransfer`. Reproduced today, on this branch:
```
evaluator: checkObjectPermission('transfer', 'crm_line') with objects.crm_line = { allowTransfer: true } -> true
lint : security-master-detail-ungranted — 'detail object "crm_line" … has no object-level CRUD
grant in any permission set'
```
`allowTransfer` is authorable and enforced today through the
insert/update `owner_id` door, so the shape is reachable. ⚠️ Whether
this is a **defect** turns on whether "object-level CRUD" is meant to
include the transfer bit — that is a ruling about the lint rule's
intent, ⛔ not a refactor, and the docblock's mirror claim is the part
that is measurably false either way. Proposed class **(b)** — a declared
claim the measurement contradicts. `Seam: spec:allowTransfer →
runtime:packages/lint/src/validate-security-posture.ts#grantsObjectAccess`.
Severity is advisory only: the rule emits `warning` and does not gate
the build. ⛔ Not touched here — outside this card's file surface, and
that exact file is held by open PR #19486. **Carrier: PR #19486**, which
already edits `validate-security-posture.ts`. Dedupe words:
`grantsObjectAccess` · `allowTransfer` ·
`security-master-detail-ungranted` · `validate-security-posture` ·
`object-level CRUD`.
**② The wildcard-super-user question is stated twice, and both agree —
noted, not filed.** `resolveObjectPermission` (plugin-security) and
`wildcardGrantsSuperRead`
(`packages/plugins/plugin-hono-server/src/current-user-endpoints.ts`)
both decide "does the `'*'` entry carry the super-user read bypass".
That is a **different** question from this card's verb fold, the
hono-server copy already documents itself as the single reading for its
own file and cites the evaluator, and they agree today. Recorded as the
boundary of what this card converged, ⛔ not widened into it. Carrier:
none (承接者:无).
**③ Three sites in `permission-evaluator.ts` still read `viewAllRecords
|| modifyAllRecords` inline, and correctly so.** `getEffectiveScope` and
`getDeclaredScope` answer the **depth** axis, and `superuserBypassSets`
answers "is the bypass bit held" — already the one function every bypass
consumer folds through, per #4647. None of them answers "does this entry
grant this verb", so none was converged. Stated so the next reader does
not mistake the omission for an oversight. Carrier: none (承接者:无).
**④ A stale code quotation in a file outside this surface — noted, not
filed.** `packages/spec/src/security/permission.test.ts` line 1172
quotes the evaluator's now-removed inline expression `permKey ===
'allowRead' && …` in a comment. Prose drift only; the assertions around
it are unaffected and still green. ⛔ Not a defect class. Carrier: none
(承接者:无).
**⑤ On the recorded affinity with #18783** — the dispatch asked to say
so if I think they are one card. **I do not.** Triage's reading holds up
from inside the work: this card needed a *measurement* and could be
finished the moment the differential came back zero; #18783 is waiting
on a *decision*. Merging them would have parked a finished measurement
behind an open question. #18783 remains open and untouched. ⛔ Merging
cards is triage's call in any case.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent d76facf commit ef256e6
5 files changed
Lines changed: 372 additions & 22 deletions
File tree
- .changeset
- packages
- lint/src
- plugins/plugin-security/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
19 | 24 | | |
20 | 25 | | |
21 | 26 | | |
| |||
39 | 44 | | |
40 | 45 | | |
41 | 46 | | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
42 | 59 | | |
43 | 60 | | |
44 | 61 | | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
49 | 66 | | |
50 | 67 | | |
51 | 68 | | |
| |||
Lines changed: 177 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
0 commit comments