|
| 1 | +# Sentinel Roadmap |
| 2 | + |
| 3 | +Current version: **`0.1.0-alpha.3`** · No GitHub release cut yet · 96.19% coverage |
| 4 | + |
| 5 | +This document is the path from the current alpha to v1, and from v1 to the v1.1 |
| 6 | +that lets Sentinel audit the Nano Collective's own infrastructure. It is written |
| 7 | +to be executed in order, with enough specificity that each item can be picked up |
| 8 | +without rediscovering the problem first. |
| 9 | + |
| 10 | +Context for why v1.1 matters beyond this repo: |
| 11 | +[`ops-scaling-strategy.md`](https://github.com/Nano-Collective/docs) in the docs |
| 12 | +repo. In short — the collective has decided Sentinel is the tool that will check |
| 13 | +every repo in the org for conformance, rather than standing up a separate |
| 14 | +checker. That makes Sentinel's own correctness an operational dependency. |
| 15 | + |
| 16 | +--- |
| 17 | + |
| 18 | +## Where things stand |
| 19 | + |
| 20 | +| | Count | |
| 21 | +|---|---| |
| 22 | +| Open PRs | 3 (all `addyCooks`, 22–24 Aug, none draft) | |
| 23 | +| Open issues | 11 — 7 bugs, 4 features | |
| 24 | +| Releases cut | **0** | |
| 25 | +| Coverage | 96.19% | |
| 26 | + |
| 27 | +Release plumbing is already in place: `release.yml` triggers on push to `main` |
| 28 | +and publishes when `package.json`'s version is ahead of npm, moving the `latest` |
| 29 | +tag onto each prerelease until a stable version claims it (that was the whole |
| 30 | +content of `alpha.3`). **Cutting a release is a version bump plus a changelog |
| 31 | +entry** — there is no changesets ceremony in this repo, and none is needed |
| 32 | +before v1. |
| 33 | + |
| 34 | +--- |
| 35 | + |
| 36 | +## Phase 0 — Merge what is already open → `0.1.0-alpha.4` |
| 37 | + |
| 38 | +Every open PR closes an open issue. This is the cheapest progress available and |
| 39 | +it takes the backlog from 11 issues to 8. |
| 40 | + |
| 41 | +| PR | Closes | Substance | |
| 42 | +|---|---|---| |
| 43 | +| **#12** surface aged / suppressed / override counts in the run summary | **#11** | `ReconcileResult` already carried `incremented`, `suppressed` and `suppressedByOverride`; the CLI and the persisted run record dropped them, so a pack author calibrating suppressions could not tell whether `sentinel:false-positive` markers were doing anything | |
| 44 | +| **#13** reject non-integer and out-of-range line numbers | **#6** | `validateLineRange` gated on `typeof === 'number'`, letting `Infinity`, `NaN` and fractional values through — `start < 1 \|\| end < start` is false for all of them, so a hallucinated `line_range` validated cleanly | |
| 45 | +| **#14** `sentinel estimate` + per-run model instrumentation | **#1** (partial) | Enhancement 1 of #1. Incremental scanning (enhancement 2) is deliberately excluded — it needs schema sign-off on the cache | |
| 46 | + |
| 47 | +**Order:** #13 first (it is a correctness fix in the findings path that the other |
| 48 | +two do not touch), then #12, then #14. |
| 49 | + |
| 50 | +**Then cut `0.1.0-alpha.4`:** bump `package.json`, write the `CHANGELOG.md` |
| 51 | +entry in the existing voice (what changed and why it mattered, not a commit |
| 52 | +list), merge to `main`. `release.yml` does the rest. |
| 53 | + |
| 54 | +**Also in this release:** issue **#9** — a stale header comment claims |
| 55 | +`sentinel run` is unimplemented. It is a one-line docs fix and it is actively |
| 56 | +misleading, so it should not wait for a later phase. |
| 57 | + |
| 58 | +--- |
| 59 | + |
| 60 | +## Phase 1 — The error-surfacing class → `0.1.0-alpha.5` |
| 61 | + |
| 62 | +**This is the most important work in the roadmap.** Four of the seven open bugs |
| 63 | +are the same failure mode: an error is detected, collected, and then silently |
| 64 | +discarded. Fix them as one change, not four. |
| 65 | + |
| 66 | +The reason this is a priority rather than tidiness: Sentinel is being given the |
| 67 | +job of reporting whether an organisation's repos are correctly configured. The |
| 68 | +failure mode of an auditing tool that swallows errors is **a green report over a |
| 69 | +broken estate** — worse than having no tool, because it is trusted. The product |
| 70 | +needs "if something went wrong, you will hear about it" as a structural |
| 71 | +property before it can hold that job. |
| 72 | + |
| 73 | +### #4 — `packLoadErrors` collected but never surfaced |
| 74 | + |
| 75 | +`source/run/run.ts:82` declares `packLoadErrors: PackLoadError[]` on the result |
| 76 | +type and `:235` populates it with `loaded.errors`. **Nothing ever reads it.** It |
| 77 | +is returned from `run()` and neither `cli.ts` nor `run/report.ts` renders it. A |
| 78 | +rule pack that fails to parse is silently absent from the audit. |
| 79 | + |
| 80 | +*Fix:* render `packLoadErrors` in the run report, and make a non-empty list |
| 81 | +visible in the CLI summary. |
| 82 | + |
| 83 | +### #5 — Dependency errors swallowed into `missingPacks` |
| 84 | + |
| 85 | +`source/run/run.ts:137–150`: |
| 86 | + |
| 87 | +```ts |
| 88 | +const resolved = resolveDependencies(loaded.packs, name); |
| 89 | +if (resolved.errors.length > 0) { |
| 90 | + missingPacks.push(name); // ← a resolution failure, reported as "missing" |
| 91 | + continue; |
| 92 | +} |
| 93 | +``` |
| 94 | + |
| 95 | +`source/run/report.ts:56` then renders that list as |
| 96 | +`> Missing packs (not in rule-packs/): …`. So a pack that **exists** but has a |
| 97 | +broken dependency graph is reported to the user as not being in `rule-packs/` — |
| 98 | +which is not merely unhelpful, it is false, and it sends the reader to look for |
| 99 | +a file that is sitting right there. |
| 100 | + |
| 101 | +*Fix:* separate the two states. `missingPacks` keeps its meaning; add a distinct |
| 102 | +channel carrying `resolved.errors` so the report can say what actually failed. |
| 103 | + |
| 104 | +### #7 — `ensureLabels` silently drops gh CLI failures |
| 105 | + |
| 106 | +`source/issues/gh-client.ts:167–174`: |
| 107 | + |
| 108 | +```ts |
| 109 | +async ensureLabels({repo, labels}): Promise<void> { |
| 110 | + // Best effort: a label that already exists or a transient failure must not |
| 111 | + // abort the run — filing tolerates a missing label per issue. |
| 112 | + for (const label of labels) { |
| 113 | + runGh(buildGhLabelArgs(repo, label)); // ← return value discarded |
| 114 | + } |
| 115 | +} |
| 116 | +``` |
| 117 | + |
| 118 | +The design intent in the comment is right — this genuinely should not abort a |
| 119 | +run. But the result is discarded entirely, so there is no record that anything |
| 120 | +failed. Note that `listIssues`, twelve lines below, checks `result.status !== 0` |
| 121 | +and throws: the codebase already has the pattern, this call site just does not |
| 122 | +use it. |
| 123 | + |
| 124 | +*Fix:* keep best-effort semantics, collect the failures, and surface them in the |
| 125 | +run summary. "Three labels could not be created" is a useful sentence; silence |
| 126 | +is not. |
| 127 | + |
| 128 | +### #8 — `readFileSync(configPath)` unhandled |
| 129 | + |
| 130 | +`source/cli.ts:240`: |
| 131 | + |
| 132 | +```ts |
| 133 | +const parsed = parseConfig(readFileSync(configPath, 'utf8')); |
| 134 | +if (!parsed.valid || !parsed.config) { |
| 135 | + for (const error of parsed.errors) { |
| 136 | + console.error(`config error — ${error.field}: ${error.message}`); |
| 137 | + } |
| 138 | + return 1; |
| 139 | +} |
| 140 | +``` |
| 141 | + |
| 142 | +There is a clean error path immediately below — and a missing or unreadable |
| 143 | +`sentinel.yaml` never reaches it, because the `readFileSync` throws first and |
| 144 | +the user gets a raw ENOENT stack trace. This is the **first thing a new user |
| 145 | +hits** if they run `sentinel run` outside a configured directory. |
| 146 | + |
| 147 | +*Fix:* wrap the read and route failures through the same `config error —` |
| 148 | +reporting path. |
| 149 | + |
| 150 | +**Then cut `0.1.0-alpha.5`.** This release is worth describing in the changelog |
| 151 | +as a class of fix rather than four bullets: errors are now surfaced rather than |
| 152 | +swallowed. |
| 153 | + |
| 154 | +--- |
| 155 | + |
| 156 | +## Phase 2 — Remaining correctness → `0.1.0-alpha.6` |
| 157 | + |
| 158 | +### #2 — `prepareRepo` accepts stale / partial clone directories |
| 159 | + |
| 160 | +`source/run/clone.ts:33–35`: |
| 161 | + |
| 162 | +```ts |
| 163 | +if (existsSync(dir)) { |
| 164 | + return {ok: true, skipped: true}; |
| 165 | +} |
| 166 | +``` |
| 167 | + |
| 168 | +Any directory that exists counts as a valid checkout. An empty directory, a |
| 169 | +half-finished clone from an interrupted run, or a stale checkout of a previous |
| 170 | +revision all return `ok: true`. The audit then runs against whatever is there |
| 171 | +and reports success. |
| 172 | + |
| 173 | +For an auditing tool this is the most consequential bug in the backlog after the |
| 174 | +error-surfacing class — it produces **findings against stale source, presented |
| 175 | +as current**. |
| 176 | + |
| 177 | +*Fix:* verify the directory is a git checkout (`.git` present), that it is not |
| 178 | +empty, and ideally that its remote matches the requested repo. On mismatch, |
| 179 | +either re-clone or return `ok: false` with a clear reason. |
| 180 | + |
| 181 | +*Note:* `clone.ts` sits inside a `/* c8 ignore */` block, so the 96.19% coverage |
| 182 | +figure does not cover this file. Whatever fix lands here needs tests that |
| 183 | +actually run. |
| 184 | + |
| 185 | +### #3 — `--rule-pack` documented as repeatable but only one is read |
| 186 | + |
| 187 | +`source/cli.ts:218` reads `flagStr(flags, 'rule-pack')` — a single value — |
| 188 | +while the help text at `:130` presents it as the mechanism for choosing packs in |
| 189 | +local mode. |
| 190 | + |
| 191 | +*Fix:* collect repeated occurrences into an array and run all of them, or |
| 192 | +correct the documentation. Prefer the former; running two packs locally is a |
| 193 | +reasonable thing to want. |
| 194 | + |
| 195 | +**Then cut `0.1.0-alpha.6`** — or roll this phase into the v1 release if it |
| 196 | +lands quickly, since only two items remain. |
| 197 | + |
| 198 | +--- |
| 199 | + |
| 200 | +## Phase 3 — v1 |
| 201 | + |
| 202 | +All 7 bugs and all 3 PRs are done. Four things stand between that and `1.0.0`. |
| 203 | + |
| 204 | +### 3a. Publish the whitepaper |
| 205 | + |
| 206 | +`README.md:9` and `docs/index.md:47` both state that this repository |
| 207 | +"describe[s] the v1 design settled in the [Sentinel whitepaper]" and link to |
| 208 | +`https://docs.nanocollective.org/collective/whitepapers/sentinel`. |
| 209 | + |
| 210 | +**That URL returns 404.** There is no `sentinel.md` in the docs repo's |
| 211 | +`content/collective/whitepapers/` directory — the only "Sentinel" string there |
| 212 | +is a frontmatter *example* in `index.md`. The document defining v1 scope is not |
| 213 | +published, while two user-facing pages send readers to it. |
| 214 | + |
| 215 | +**Decided:** publish it, with status `Building` rather than opening a 30-day |
| 216 | +public review window. Sentinel is already built; a review window for a shipped |
| 217 | +design would be theatre, but a published scope document that v1 can be checked |
| 218 | +against is worth having — particularly now the collective's own conformance |
| 219 | +checking depends on this product. |
| 220 | + |
| 221 | +Flip the status to `Shipped` when `1.0.0` lands. |
| 222 | + |
| 223 | +Two details to settle when writing it: |
| 224 | + |
| 225 | +- The frontmatter carries `review_opens` and `review_closes`, described in the |
| 226 | + docs index as driving the status badge. A retro-published whitepaper has no |
| 227 | + review window, so either omit them or record the dates the design was actually |
| 228 | + settled. Check how the badge renders when they are absent. |
| 229 | +- `proposer: "Will Lamerton"`, `proposer_github: "will-lamerton"`. |
| 230 | + |
| 231 | +### 3b. #10 — Enforce `severity_weighting` |
| 232 | + |
| 233 | +`severity_weighting` is parsed from the manifest into |
| 234 | +`manifest.severityWeighting` (`source/rule-packs/types.ts:31`) and passed into |
| 235 | +the prompt by `buildAuditPrompt` — but nothing compares the model's emitted |
| 236 | +severity against it. A pack declaring |
| 237 | +`severity_weighting.missing-signer-check: critical` can have the model emit |
| 238 | +`low` and pass validation cleanly. |
| 239 | + |
| 240 | +The feature's entire purpose is making severity authoritative per pack, and it |
| 241 | +is currently advisory in practice while reading as enforced. |
| 242 | + |
| 243 | +*Fix:* after `validateFindings` returns, walk the surviving findings and, for |
| 244 | +each `finding.rule` with a key in the active pack's `severityWeighting`, |
| 245 | +**overwrite** `finding.severity` with the manifest value — "the pack's word is |
| 246 | +law." |
| 247 | + |
| 248 | +The issue records an alternative (reject the finding with a validation error on |
| 249 | +mismatch). Overwriting is simpler and does not punish an otherwise accurate |
| 250 | +finding with a retry, so prefer it — but whichever lands must be documented, |
| 251 | +because the two behaviours are indistinguishable to a pack author until one |
| 252 | +fires. |
| 253 | + |
| 254 | +### 3c. #1 (second half) — Incremental scanning |
| 255 | + |
| 256 | +The half deliberately excluded from PR #14, pending sign-off on the cache |
| 257 | +schema. Rerunning a full audit when a handful of files changed is the dominant |
| 258 | +cost as Sentinel scales across an org. |
| 259 | + |
| 260 | +**The schema question, concretely.** `source/observe/types.ts` already defines |
| 261 | +`RunRecord` — a durable per-run record committed to the config repo and read |
| 262 | +back by the dashboard (`cli.ts:190`). It carries timestamp, mode, per-repo and |
| 263 | +per-pack findings, severity counts, filing totals and target errors. |
| 264 | + |
| 265 | +What it does **not** carry is scan provenance: which commit each repo was at, |
| 266 | +which file states each pack actually saw. That is precisely what incremental |
| 267 | +scanning needs. So the decision is: |
| 268 | + |
| 269 | +- extend `RunRecord` with per-repo commit SHA and per-pack file-state |
| 270 | + provenance, and let the existing durable store double as the cache; or |
| 271 | +- introduce a separate cache artifact and leave `RunRecord` as a reporting |
| 272 | + surface. |
| 273 | + |
| 274 | +Prefer the first — there is already a committed, versioned per-run store, and a |
| 275 | +second one invites the two drifting apart. |
| 276 | + |
| 277 | +**Add a `schemaVersion` field to `RunRecord` in the same change.** It does not |
| 278 | +have one today, and the dashboard reads every historical record back. Once |
| 279 | +records exist in two shapes with nothing distinguishing them, the reader has to |
| 280 | +guess. This costs one field now and a migration later. |
| 281 | + |
| 282 | +**This is why doing it before `1.0.0` is the right call.** `RunRecord` is a |
| 283 | +committed artifact with a stability expectation the moment a stable version |
| 284 | +ships. Settling its shape while still on alpha is free; changing it afterwards |
| 285 | +is a migration. |
| 286 | + |
| 287 | +One design fork to settle (see open questions): git-diff-based change detection |
| 288 | +is simplest but assumes git history is present, which `--no-clone` and local |
| 289 | +mode may not guarantee. Content hashing works everywhere but reads every file. |
| 290 | + |
| 291 | +### 3d. Cut v1 |
| 292 | + |
| 293 | +Bump to `1.0.0`. `release.yml` already handles the prerelease → stable |
| 294 | +transition: the `latest` tag stops being force-moved and is claimed by the |
| 295 | +stable version naturally. |
| 296 | + |
| 297 | +Before tagging, re-audit the README's "where something is planned rather than |
| 298 | +shipped, the docs say so" caveat against what v1 actually contains, and flip the |
| 299 | +whitepaper status to `Shipped`. |
| 300 | + |
| 301 | +--- |
| 302 | + |
| 303 | +## Phase 4 — v1.1: auditing the collective |
| 304 | + |
| 305 | +v1.1 is where Sentinel takes on the job described in the ops strategy. Two |
| 306 | +additions, both natural extensions of what the product already does. |
| 307 | + |
| 308 | +### 4a. The conformance rule pack |
| 309 | + |
| 310 | +A rule pack that audits repository *configuration* rather than source. Reports |
| 311 | +only — it opens issues, it does not open PRs or change settings. For each repo |
| 312 | +in the organisation: |
| 313 | + |
| 314 | +- calls the shared `pr-checks` reusable workflow from `Nano-Collective/.github` |
| 315 | +- has the full `test:*` script set (`lint`, `types`, `format`, `ava`, `knip`, |
| 316 | + `audit`, `security`, `all`) |
| 317 | +- coverage >= 80% with fail-on-drop enabled |
| 318 | +- `CODEOWNERS` exists and names at least two owners |
| 319 | +- both org rulesets applied with the expected parameters |
| 320 | +- changesets and `release-prepare.yml` present |
| 321 | +- `CONTRIBUTING.md`, `LICENSE`, `MAINTAINING.md` present |
| 322 | +- no direct collaborator entries — access is team-based only |
| 323 | +- live team membership matches the committed `teams.yml` |
| 324 | + |
| 325 | +This is a genuine extension of the rule-pack model: existing packs read source |
| 326 | +files, this one reads repository metadata via `gh`. Expect it to need a new |
| 327 | +source type alongside `RepoFiles`. |
| 328 | + |
| 329 | +Until this ships, the collective has no drift monitoring — an interim check |
| 330 | +rides along with the org-wide stale-escalation cron and retires when this lands. |
| 331 | + |
| 332 | +### 4b. PR review commentary |
| 333 | + |
| 334 | +Sentinel's v1 scope says it files issues and does not comment on pull requests. |
| 335 | +The collective needs a PR review agent (`nc-review`), and building a second |
| 336 | +Nanocoder-driven review tool alongside Sentinel would be duplication. |
| 337 | + |
| 338 | +The v1.1 extension: allow a rule pack to target an open PR's diff and post a |
| 339 | +structured review rather than filing an issue. The rubric is process-focused — |
| 340 | +follows `CONTRIBUTING`, changeset present, tests present, duplicates an open PR, |
| 341 | +scope crept beyond the linked issue — deliberately excluding anything CI already |
| 342 | +checks, since required status checks cover correctness. |
| 343 | + |
| 344 | +**It must never merge.** Labelling (`agent:clean` / `agent:needs-work`) and |
| 345 | +commentary only. |
| 346 | + |
| 347 | +## Summary |
| 348 | + |
| 349 | +| Phase | Release | Contents | |
| 350 | +|---|---|---| |
| 351 | +| **0** | `0.1.0-alpha.4` | Merge PRs #13, #12, #14 → closes #11, #6, #1(partial). Plus #9 docs fix | |
| 352 | +| **1** | `0.1.0-alpha.5` | Error surfacing as one change: #4, #5, #7, #8 | |
| 353 | +| **2** | `0.1.0-alpha.6` | #2 clone validation, #3 repeatable `--rule-pack` | |
| 354 | +| **3** | **`1.0.0`** | Whitepaper published, #10 severity enforcement, #1b incremental scanning + cache schema, release cut | |
| 355 | +| **4** | `1.1.0` | Conformance rule pack, PR review commentary | |
| 356 | + |
| 357 | +The critical path runs through phase 1. Everything after it depends on Sentinel |
| 358 | +being a tool that tells you when something went wrong. |
| 359 | + |
| 360 | +Phase 3 is the longest phase by a distance — it carries both remaining features |
| 361 | +plus the `RunRecord` schema decision. That is a deliberate trade: the schema |
| 362 | +becomes expensive to change the moment `1.0.0` ships, so it is settled while |
| 363 | +still on alpha. |
| 364 | + |
| 365 | +--- |
| 366 | + |
| 367 | +## Open design questions |
| 368 | + |
| 369 | +**Incremental scanning: how is change detected?** (blocks 3c) |
| 370 | + |
| 371 | +Git-diff against the previous run's recorded commit is the simplest and cheapest |
| 372 | +approach, but it assumes git history is present — `--no-clone` and local |
| 373 | +single-pack mode may hand Sentinel a directory with no `.git`, and `prepareRepo` |
| 374 | +clones are shallow (`--depth 1` style), so history depth is not guaranteed |
| 375 | +either. |
| 376 | + |
| 377 | +Content hashing per file works regardless of source and degrades gracefully, but |
| 378 | +reads every file on every run — which removes some of the saving the feature |
| 379 | +exists to deliver, though it still avoids the expensive part (the model calls). |
| 380 | + |
| 381 | +A hybrid — git diff where a usable checkout exists, hashes otherwise — is |
| 382 | +probably right, but it means the cache carries two provenance shapes and the |
| 383 | +`schemaVersion` field has to accommodate both from day one. |
| 384 | + |
| 385 | +**`severity_weighting` on mismatch: overwrite or reject?** (3b) |
| 386 | + |
| 387 | +Overwrite is recommended above and in the issue. Worth confirming, because the |
| 388 | +two behaviours are indistinguishable to a pack author until one fires, and the |
| 389 | +choice has to be documented either way. |
0 commit comments