feat(estimate): add sentinel estimate and per-run model instrumentation - #14
feat(estimate): add sentinel estimate and per-run model instrumentation#14addyCooks wants to merge 3 commits into
sentinel estimate and per-run model instrumentation#14Conversation
…tion Answers the first half of Nano-Collective#1: how long an audit will take and roughly what it will cost, before it runs. `sentinel estimate` reports repositories, rule packs, files, model requests, tokens, and wall-clock runtime for a config, without invoking a model, filing anything, or mutating anything. The token figure is measured rather than guessed — the estimator assembles the same prompts the audit would send, via the same `buildAuditPrompt` and `applies_to` scoping, and counts them. What varies between installs is the per-request cost, so every run is now instrumented for calibration: each pack pass is timed, and its prompt and output tokens counted, into a `usage` block on the committed run record (requests, duration, prompt and output tokens). The estimator averages the last ten records to derive its per-request figures, and says so in the output when it is still on built-in defaults. Repos already checked out under the workspace are measured from their real files; `--clone` checks out the rest. A repo that contributed no files, a missing or unparseable pack, and a target that would not expand are each called out so a partial estimate never reads as the whole picture. Also extracts the target's pack resolution out of `runFromConfig` into `selectPacks`, so the estimate and the run agree on what would execute. No versioned contract is touched: the findings model, sentinel.yaml schema, and pack manifest are unchanged, and `usage` is optional on the run record so records written before this land still read.
|
Estimation half is up. Before I start the incremental side one thing to settle, and it's bigger than Incremental scanning breaks auto-resolution today. for (const [hash, issue] of openByHash) {
if (findingByHash.has(hash)) continue;
const misses = readMisses(issue.body) + 1;
if (misses >= resolveAfterMisses) plan.toResolve.push(issue);
}So skipping unchanged files would quietly close real, unfixed findings after What we can do is, pass the scanned scope into the planner so an out-of-scope issue is held, The rest: Per-repo config? incremental: true|false per target, off by default until it's proven somewhere real. @akramcodez wdyt? |
will-lamerton
left a comment
will-lamerton
left a comment
There was a problem hiding this comment.
Really strong PR. The scope discipline is right - splitting out incremental scanning because it needs schema sign-off, and keeping this half free of any versioned contract, is exactly the call I'd have wanted. And sharing selectPacks and buildAuditPrompt between the estimator and the runner is the detail that makes the whole thing trustworthy: the two paths can't drift on what would actually execute, which is what turns the token figure into a measurement rather than a model of one. I diffed the extracted selectPacks against the inlined original and the logic and ordering are identical.
I ran the full gate on fa8bfa6: tsc --noEmit clean, biome check clean, 299 tests pass, knip clean, estimate.ts at 100% line / 98% branch coverage. So the unchecked "pnpm test:all passes locally" box is a false negative - it does pass.
Requesting changes on two things, because both make the headline numbers wrong in the same direction (understating cost), and both are cheap to fix with data the PR already collects.
1. Output tokens are undercounted on retries, and this feeds calibration
source/run/audit.ts:
promptTokens: estimateTokens(prompt) * result.attempts,
outputTokens: estimateTokens(result.raw),result.raw is only the final attempt's output, so a 2-attempt pass records prompt x2 but output x1.
That isn't just an observability gap. calibrate computes outputTokensPerRequest = outputTokens / requests, and requests includes retries, so every retried pass drags the per-request output figure down and estimate then understates output tokens for everyone. The asymmetry looks unintentional: the prompt line has a comment justifying the * attempts, the output line has none, and your counts the prompt once per attempt test asserts the prompt side while staying silent on the output side.
Cheapest fix consistent with the existing approximation is estimateTokens(result.raw) * result.attempts with a comment matching the one above it. Better, if runAuditWithAutoFix can accumulate raw output across attempts, count it exactly.
2. Runtime is calibrated per-request, but prompt size is the thing that varies
In estimateRun, durationMs = requests * msPerRequest, where msPerRequest is a flat average across prior runs. That's prompt-size-blind, but the docs pitch estimate at exactly the case where prompt size changes:
useful when you are about to point Sentinel at a dozen more repositories
Calibrate on a 2.5K-token config, estimate a 500K-token one, and the runtime figure is off by roughly the ratio, in the direction that matters. For a local model, decode time tracks token count fairly directly, so this is a real error rather than a rounding one - and runtime is the figure people will actually schedule against.
The good news is the data to fix it is already being recorded. RunUsage carries both durationMs and promptTokens, so an ms-per-prompt-token figure (or a fixed msPerRequest plus a msPerToken term) is derivable from the same records with no schema change. Given the framing of this PR is "measured, not guessed", I'd rather do that now than ship the one figure that stays a guess. If you'd rather defer it, the docs need to say the runtime figure assumes prompt sizes comparable to your recorded runs.
3. The --clone warning fires for repos that are already present
caveats() selects on repo.files === 0, but repo.files is audited.size - the count after applies_to scoping. A checked-out repo that legitimately matches no files (a Python pack pointed at a TypeScript repo) gets told to clone something it already has, which sends the operator looking for the wrong problem.
You already have the pre-scoping count in the loop (files.length before buildAuditPrompt). Carrying it on RepoEstimate lets you split the two messages: "not checked out" vs "no files matched the packs' applies_to". The second is arguably the more useful of the two, since it means a pack is configured against a repo it can't see.
4. Docs contradict the --clone flag
Both docs/cli/index.md and ESTIMATE_USAGE say the command "runs no model, files nothing, and mutates nothing", then document a flag that clones repositories onto disk. Reword to something like "runs no model and files no issues; --clone will check out missing repos". Small, but it's the sentence someone reads before deciding the command is safe to run.
Breaking change isn't called out
PackOutcome.usage is required and PackOutcome is exported from source/index.ts, so any external consumer constructing one breaks at compile time - report.spec.ts had to be updated for exactly this reason. On alpha.3 that's completely fine, but the "Breaking changes are called out above" box is ticked and nothing is called out. Worth a line in the description, and a CHANGELOG entry when it releases.
Nits, take or leave
runEstimateprintstargetErrorsto stderr and they're already rendered into the Markdown caveats, so a stdout run shows them twice. It also always returns 0 even when every target failed to resolve. Fine for an advisory command, but worth making that a deliberate call rather than a default.formatDuration: 89s renders as "89 second(s)", 90s as "2 minute(s)". Cosmetic.readFileSync(configPath)is unguarded, so a missingsentinel.yamlthrows a raw ENOENT stack. This matches whatrunRunalready does, so it's consistent with the codebase - flagging only so that if we ever fix one we fix both.
Nothing else is risky. RunRecord.totals.usage being optional with calibrate skipping records that lack it is the right call for backwards compatibility, and fsRepoFiles.read returns [] on a missing directory rather than throwing, so the un-cloned path degrades cleanly.
Fix 1 and 2 and I'm happy to merge. 3 and 4 are quick, the rest can ride along.
Review follow-ups. Both blocking items understated cost in the same direction, and both are fixed from data the run records already carry. Output tokens counted only the final attempt while prompt tokens were multiplied by the attempt count, so a retried pass recorded prompt x2 and output x1. Because calibrate divides output by a request count that includes retries, every retry dragged outputTokensPerRequest down and estimate then understated output for everyone. runAuditWithAutoFix now reports promptChars and outputChars accumulated across attempts, so both sides are exact rather than the final attempt scaled up: the retry's prompt carries a correction section the x2 approximation missed, and its discarded first response cost tokens to generate. Characters rather than tokens keeps the approximation in one place, and keeps whole prompts from being retained on the result. Runtime was requests x a flat msPerRequest, which is prompt-size-blind — the exact case the command is pitched at. Calibration now carries a fixed msPerRequest plus a marginal msPerPromptToken, least-squares fitted across records when they differ in prompt size. When they cannot separate the terms — one record, or every run the same size — the measured average is split using the proportion the defaults imply, so the magnitude stays measured even where the shape is assumed. A negative fitted term falls back the same way. The defaults still sum to the previous 45s at a 2,500-token prompt. Also: - The --clone warning selected on the post-applies_to count, so a repo that was checked out but matched no files was told to clone what it already had. RepoEstimate carries the pre-scoping count and the two cases now get separate warnings; the second is the more useful, since it means a pack is pointed at a repo it cannot see. - Docs and --help claimed the command "mutates nothing" while documenting --clone. Reworded, and the runtime model is documented. - formatDuration handed over to minutes at 90s while rounding minutes from 60s, so "1 minute(s)" was unreachable: 89s rendered as seconds and 90s jumped to 2 minutes. - runEstimate printed targetErrors to stderr even when the report went to stdout, where the caveats already list them. Now only when --output redirects the report to a file. The 0 exit is left as it was, with a comment marking it deliberate rather than an oversight. Left alone: the unguarded readFileSync(configPath). It matches runRun, so fixing one without the other would just make the two inconsistent. Breaking change, from the original commit rather than this one: PackOutcome.usage is required and PackOutcome is exported from source/index.ts, so an external consumer constructing one breaks at compile time.
|
Hi @will-lamerton ! All four sorted, plus two nits.
Nits: Breaking change is in the description now; CHANGELOG at release, since it's only touched by
Heads up: Want me to open the incremental-scanning issue? |
will-lamerton
left a comment
will-lamerton
left a comment
There was a problem hiding this comment.
Verified 0d5a45f locally: tsc --noEmit clean, biome check clean, 309 tests pass.
1, 2 and 4 are done, and the first two are done better than I asked. Taking the exact option on output tokens was the right call, and catching that the prompt side was low too (the correction section the retry sends) is the part I'd missed. The runtime fit is consistent end to end: calibrate fits per-request ms against per-request prompt tokens and estimateRun applies requests * fixed + totalPromptTokens * marginal, which is the same model. The non-negativity guard also caps the fitted slope at the pooled ms-per-token, so a two-record exact fit cannot extrapolate into nonsense. Nits all handled, and leaving readFileSync alone is the right call.
Requesting changes on one thing.
3 is not fixed, and the test cannot see it
filesPresent comes from files.length, but files is:
const files = await deps.files.read(repoDir, unionPatterns(packs));fsRepoFiles.read already filters by those patterns (sources.ts:68-73), so filesPresent is a post-applies_to count too, not the pre-scoping one.
The exact case from my last review still misfires. Running estimateRun against a real checked-out TypeScript repo with a **/*.py pack, using the real fsRepoFiles, gives filesPresent=0, files=0 -> still "not checked out", still told to pass --clone.
The new test passes because the repoFiles stub (estimate.spec.ts:56-64) ignores its patterns argument and returns everything, so it models a read production never performs. What the split does catch today is only where the union is broader than one pack's own matching: a multi-pack target where some other pack matched, or a pack with empty paths where the union becomes everything and per-pack matchesAppliesTo then excludes it all. Single-pack mismatch, the case I raised, stays wrong.
Fix: source filesPresent independently of the patterns, either a second deps.files.read(repoDir, []) or an existence check on repoDir. And make the stub honour patterns, otherwise the test cannot tell the two states apart.
Smaller
AutoFixResultgained requiredpromptChars/outputCharsand is exported fromsource/index.ts, so it is the same class of compile-time break asPackOutcome.usage. Worth adding to the description alongside it.- Your
tsconfig.jsonheads-up is right and worth its own issue:test:typesexcludessource/**/*.spec.ts, so the gate cannot catch exactly the breakage we are calling out.
Fix 3 and I will merge. Yes to opening the incremental-scanning issue, and yes to the plan in your earlier comment: passing scanned scope into planReconciliation so an out-of-scope issue is held rather than missed is the part that has to land in the same PR as the cache.
The previous fix did not work. `filesPresent` came from `files.length`,
but that read is already scoped:
const files = await deps.files.read(repoDir, unionPatterns(packs));
fsRepoFiles.read filters by the patterns it is given, so the count was
post-applies_to too. A checked-out TypeScript repo with a single `**/*.py`
pack still produced zero, so it was still reported as not checked out and
still told to clone what it already had. Only the cases where the union is
broader than one pack's own matching — a multi-pack target where another
pack matched, or an empty `paths` making the union everything — happened
to work.
`RepoEstimate.checkedOut` replaces the count with what the warning
actually needs to know, and the probe is a second read with no patterns.
That read runs only when the scoped one came back empty: with files in
hand the repo is obviously present, so the extra read is skipped on every
path where it could not change the answer.
The test could not see any of this. The repoFiles stub ignored its
patterns argument and returned everything, modelling a read production
never performs, so a repo that matched nothing still arrived with files.
The stub now applies patterns exactly as fsRepoFiles does.
Both halves were needed: with the old stub, the broken implementation
passes the whole suite, including the test written to catch it.
Covers the case from review directly — one pack scoped to a language the
repo does not contain — plus the absent-checkout counterpart, and a test
pinning that the unscoped read is skipped when the scoped one found files.
|
Hey @will-lamerton, Sourced it independently of the patterns Ran it end to end against real
|
2 participants
Description
Implements enhancement 1 of #1 audit cost and runtime estimation, plus the
instrumentation that makes those figures real rather than a constant.
Enhancement 2 (incremental scanning) is deliberately not here: it needs schema
sign-off on the cache, and this half touches no versioned contract, so it can
land independently.
The command.
sentinel estimatecovers every factor the issue lists:Flags:
--config --packs-dir --workspace --records-dir --clone --output.Why the numbers hold up. Three things could have made this a guess, and all
three are addressed:
buildAuditPromptwith the realapplies_toscoping anddepends_onresolution, so it counts the prompt themodel would actually receive. Verified: a live
runrecordedpromptTokens: 2501, and the estimator independently counted the same 2,501for that config.
by orders of magnitude, so a hardcoded seconds-per-request would be useless.
Every run is instrumented each pass timed, its tokens counted into
RunRecord.totals.usage, across every attempt rather than just the one thatvalidated.
calibrate()derives its figures from the last ten records. Untila run exists it falls back to built-in defaults and says so in the output,
so an uncalibrated first estimate is never mistaken for a measured one.
fixed amount plus an amount that tracks prompt size, and both terms are
least-squares fitted from the records, so sizing a config far larger than
anything you have run is not priced as though the prompts stayed the same.
When the records cannot separate the two one run, or every run the same
size the measured average is split by the proportion the defaults imply, so
the magnitude stays measured even where the shape is assumed.
Honest partial estimates. Repos not checked out, repos that are checked out
but match no pack's
applies_to, packs missing fromrule-packs/, packs thatfail to parse, and targets that will not expand are each surfaced as a separate
warning, so an understated estimate never reads as the whole picture.
--clonechecks out what is missing.
Open questions from the issue
runbehaviour iscompletely unchanged you reach for
estimatewhen changing the config, noton every scheduled pass, and it stays free of the model entirely.
on pack changes, forcing a full audit) belong with the caching half and are
not addressed here.
Notable refactor
Target pack resolution moved out of
runFromConfigintoselectPacks(
run/select.ts) and is used by both paths, so the estimate and the run cannotdrift on what would actually execute.
run.tsis 23 lines shorter for it.Record reading in
cli.tsis likewise now shared between the dashboard and theestimate rather than duplicated.
Review follow-ups (
0d5a45f)multiplied by the attempt count. Since
calibratedivides output by a requestcount that includes retries, every retry dragged
outputTokensPerRequestdown and the estimate understated output for everyone.
runAuditWithAutoFixnow reportspromptChars/outputCharsaccumulatedacross attempts, so both sides are exact the prompt side was low too, since
prompt * attemptsmisses the correction section the retry sends.requests * msPerRequest). Split into afixed and a marginal term as described above.
--clonewarning fired for repos already present, because it selectedon the post-
applies_tocount. Two passes at this: the first carried afilesPresentcount read withunionPatterns(packs), which is itselfpattern-filtered, so it was post-scoping too and a single-pack mismatch still
misfired.
RepoEstimate.checkedOutnow answers the question the warningactually asks, probed with an unscoped read that runs only when the scoped
read came back empty — the sole case where it can change the answer. The
repoFilestest stub ignored itspatternsargument, so it modelled a readproduction never performs and could not see the bug; it now filters exactly
as
fsRepoFilesdoes.--helpclaimed the command "mutates nothing" while documenting--clone. Reworded, and the runtime model is documented.formatDurationhanded over to minutes at 90s while rounding minutes from60s, so
1 minute(s)was unreachable 89s rendered as seconds and 90s jumpedto 2 minutes.
runEstimateprintedtargetErrorsto stderr even when the report went tostdout, where the caveats already list them. Now only under
--output. The0exit is unchanged, with a comment marking it deliberate.Breaking changes
Two, both the same class: a required field added to a type exported from
source/index.ts, so an external consumer constructing one breaks at compiletime. Fine on alpha.3, but calling them out.
PackOutcome.usageis required.report.spec.tshad to be updated forexactly this reason.
AutoFixResult.promptCharsandAutoFixResult.outputCharsare required.Our own gate cannot catch either:
test:typesexcludessource/**/*.spec.ts(tsconfig.json), so
tsc --noEmitnever typechecks the specs where such abreak would first surface. Tracked separately.
A CHANGELOG entry belongs with the release commit:
CHANGELOG.mdis only evertouched by
release:commits andscripts/extract-changelog.jskeys offversion headings.
Type of change
Testing
pnpm test:allpasses locally.Checklist
sentinel.yamlschema, rule-pack manifest, findingsmodel).