docs(vrs): specify restart budgets and helper teardown - #209
Conversation
4a95d2a to
171b669
Compare
schickling-assistant
left a comment
There was a problem hiding this comment.
Read at 171b66913737234ff094e164ded7d89b06a6ad22. Taking the two questions in the order
you ranked them.
Do these describe the system, or the two PRs?
The requirements are timeless. One line in spec.md is not, and it is R32's.
R31 and R32 in requirements.md both state properties a conformant implementation must
have. Neither mentions a change, a before, or an author. They read the same in six months
as today.
spec.md R32 ends with:
PR #202 provides failure-capable evidence that removing only the group signal lets a
descendant outlive the direct child while the direct-child controls remain green.
That sentence is not about the system. It is about an experiment performed on a diff —
delete a line, observe that the suite stays green anyway. It is genuinely useful, and it is
the strongest argument that the test earns its keep, but a reader asking "what does st2
guarantee about helper teardown?" is handed a narrative about what happened when someone
removed something. That is the drift you predicted, and this is its one instance.
R31's citation is milder and I would leave it: "provides cadence, recovery, and
unobserved-pass evidence for this accounting" names evidence categories rather than
retelling a diff.
Suggestion, not a change request: keep the fact, move the narration. The durable form is
what the system does — the group signal is what reaches descendants, and direct-child
assertions cannot observe its absence. Where that was established belongs beside the
evidence, not in the guarantee.
A finding I checked and withdrew, stated because the near-miss is instructive. R31/R32
are inserted immediately before R22, which reads as out of numeric order. It is not a
defect: the file was already non-numeric — R22 precedes R05, R11 precedes R08, R27 precedes
R23. The list is grouped thematically and the insertion follows existing practice. Reading
the whole sequence rather than the insertion point is what killed it.
Is R32's helper enumeration narrower than the class?
No — and the measurement says the opposite of what you feared.
I enumerated every caller of the bounded-output helpers at this head. Production call
sites, repo-wide:
run.rs:650—pty list --jsonrun.rs:627—pty metadata patch --id
That is all of them. The remaining references are the definitions themselves and test call
sites. No other module calls either helper.
So the enumeration is not a narrow sample of a broader class; it is currently the complete
class, and such as correctly leaves it open for callers that do not exist yet. The
guarantee also attaches to the helper rather than to its callers — the session and group
setup happen inside the spawn path — so a future caller inherits it without the requirement
needing to name it.
What would have shown the opposite: any call site outside those two, or a second spawn path
with its own teardown. Neither exists at this head. The check can find things — it locates
the definition and every test site through the same query.
One thing outside your two questions, because it bears on this PR's job
R27 is assigned twice. requirements.md:153 is "Transactional catalog authoring";
requirements.md:265 is "Typed agent desired state". Two distinct requirements, one ID.
R12 and R18 do not appear at all.
This predates the PR and is not yours to fix here. It matters because this PR's work is
allocating new IDs, and a duplicated ID space makes "take the next free number" unreliable —
a cross-reference to R27 already cannot be resolved to one requirement. Worth knowing before
the next allocation rather than after.
flapping.rs
The replacement rationale is correct and the arithmetic holds: at attempts 3 / interval 60s, a task staying up longer than interval / attempts — 20s — never accumulates three
launches inside one window, so a window-scoped fail budget cannot exhaust. That is a
property of the counter, not of any cadence, which is what makes it a durable justification
where the removed one was false.
Not approving or requesting changes — a comment review, and the disposition is the author's.
agent-tool: Codex agent-tool-version: 0.145.0 agent-runtime: Codex 0.145.0 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
agent-tool: Codex agent-tool-version: 0.145.0 agent-runtime: Codex 0.145.0 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
171b669 to
0ee6216
Compare
schickling-assistant
left a comment
There was a problem hiding this comment.
Bounded confirm at 0ee6216dbbaaf908f37e424da01039ccaba64e07, on that line only, as
agreed. Confirmed — the replacement is the durable form and I have no objection.
The process-group signal reaches a descendant that outlives the direct
child; terminating the direct child alone does not.
That is a property of the system, stated without reference to a change, an author, or a
mutation. It reads the same in six months. And #202 is now cited as an evidence category
in the same shape R31 cites its own, which was the asymmetry worth removing.
Scope verified rather than assumed. The compare API is three-dot — it reports from the
merge base, so on a force-pushed branch it shows the whole PR and cannot answer "what did
the amendment change". I fetched both tips and diffed the five touched files directly:
docs/vrs/spec.md— changed, exactly the four lines abovedocs/vrs/requirements.md— unchangeddocs/vrs/02-agent-spec/spec.md— unchangeddocs/vrs/02-agent-spec/requirements.md— unchangedsrc/flapping.rs— unchanged
So "that is the whole diff" holds. Control: perturbing one byte of a fetched file makes
the differ report a difference, so an unchanged verdict is a measurement rather than a
comparison that could not fail.
Nothing else re-read, per the terms.
Why
Merged PRs #191 and #202 establish two runtime guarantees that the VRS did not yet own: fail-mode restart budgets remain reachable independently of reconcile cadence, and bounded helper teardown targets descendants as well as the direct child.
The restart-cap module documentation also still cited unrelated requirement R16 and justified the fix with a timer-only launch-rate claim, even though
st2 upreconciles from filesystem events as well as its timer.What
How and rationale
R31 distinguishes delay mode's sliding-window rate limit from fail mode's uptime-forgiven terminal budget. R32 remains in the root supervision contract because bounded helper lifecycle is generic st2 behavior, not Agent Spec field semantics.
The IDs follow the live root requirement space plus every open main-targeting PR that touches it: merged PR #77 defines R29 and R30, leaving R31 and R32 as the next non-conflicting IDs.
Verification
nix flake check --no-build— passed.cargo test --lib flapping::tests— 8 passed, 0 failed.cargo test --lib reap— 10 passed, 0 failed.git diff --check origin/main..HEAD— passed.References
attemptsis reachable #191.