This repository was archived by the owner on Oct 27, 2026. It is now read-only.
Repository navigation
Loop/args escaping - #107
Open
Dhivanujan wants to merge 140 commits into
Open
Dhivanujan wants to merge 140 commits into
Dhivanujan wants to merge 140 commits into
Conversation
Production's apps/flow is CRLF throughout (measured 18 Sep 2026, 185/185 text files). The fork is LF, and this stops a Windows checkout from reintroducing CRLF.
Adapted from veyqon-dev-scaffold: test gate, guards, loops, brain. Not for upstream.
Every prompt now carries a one-line context block on the system message: the ISO date with its weekday, the local time, the zone that time is expressed in (the user's own when set, otherwise the system's), and who is speaking. It is rebuilt per prompt and never stored, so a session left open for days reports today rather than the day it started. An unknown zone falls back to the system zone instead of being passed on: the conversion helper answers an unknown zone with the UTC time rather than raising, which would label a clock reading with a zone it is not in. The user's name is whitespace-collapsed and length-capped before it goes into the system role, so it cannot add lines of its own. Also fixes the transcript delta this exposed. What a run "produced" was sliced positionally from the stored row count, which assumed the prompt had exactly one message per stored row. A session with no stored system message now gets one for the context, and without the matching offset the last stored message was re-persisted as run output — every user message duplicated from the second turn on. The ephemeral prefix is now derived and skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ext block The new tests used the platform's freeze-time helper, which needs a package that is installed on neither CI nor a freshly built bench: bench installs apps with a bare editable install and no extras, and the workflow has no test-requirements step. Those tests passed locally only because the package had been hand-installed here, and would have errored everywhere else. Replaced with a helper that pins the instant rather than the process clock, by substituting only "what time is it now" and leaving the zone conversion beneath it real. A fixed return value would have been simpler and would have made the time-zone assertions vacuous — they would have been asserting their own stub. Also from the review pass: - The user's name is framed as data rather than prose, the way saved memories already are. It is user-editable text sitting in the highest-trust role, and a hundred characters is room enough for an imperative sentence. - An account id is no longer passed off as a name. With no name recorded the lookup answers with the account id; when that is an address it is dropped rather than handed to the model. - The zone lookup now validates the system zone too, not only the user's. Its docstring had been promising more than it delivered. - Memory delivery is now covered. The memory block and the per-turn context block were merged onto one code path with nothing asserting that both still arrive; two tests now pin that, and their order, on both branches. - A name can no longer close the quotes that hold it — the substitution existed but no test could fail on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esume path Both gaps came out of adversarial review of the change on this branch. The zone fallback was only ever tested with an obviously bogus name, so it proved nothing about the case that actually happens. The platform's own picker offers names this tzdata build cannot resolve — renamed and alias zones like Europe/Kiev, Asia/Rangoon, CET and EST, eight of which were measured failing here on 19 Sep 2026. That is the dangerous shape, because the conversion helper answers an unknown zone with the UTC time rather than raising: without the fallback the block would read a UTC clock and label it Asia/Rangoon, six and a half hours out, and wrong about the date either side of midnight. Tested now with names that occur. The system zone is covered too, not just the user's. A bad settings value would mislabel every user's clock rather than one user's. The session-level resume had no test anywhere in the suite — every existing resume test drives the agent library one layer below it. So "still current on resume after a pause" was true but ungated, and routing resume around the prompt builder would have dropped the context from every resumed run without turning anything red. The new test pauses a turn late on one day, approves it after midnight the next, and asserts the resumed prompt carries the new date, that the approval still gated the write, and that nothing dated was stored. Verified by probe, each restored from a byte backup and re-hashed: leaving the system zone unvalidated reddens one test; removing validation entirely reddens five; routing resume around the prompt builder reddens the resume test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An agent's instructions were stored as the session's system message on the first turn and never looked at again, so editing an agent changed only conversations started afterwards. Rolling out a revised prompt would silently skip every session already open, and nothing on the outside said which prompt a given conversation was running. The prompt builder now takes the system message's base text from the agent record as it is now. Ephemeral, like the per-turn context and the memory block beside it: the stored row is never rewritten, so the transcript still shows what was actually stored. Resume goes through the same builder and so picks up the current text too. A session driven by an agent defined in code keeps its stored row verbatim. It has no record to read, and its transcript is the only copy of what it was told. One boundary is worth stating rather than leaving to be discovered: the text comes from the runtime the session was loaded with, so an edit made after that load is not seen until the session is loaded again. A turn is a request and a request loads the session, so in practice the next turn is when an edit lands. Verified by probe, each restored from a byte backup and re-hashed: dropping the code-agent guard reddens the code-agent test; writing the instructions into the stored row reddens the never-rewritten test. The first attempt at that first probe stayed green, which found a weak test rather than a bug — a code session's runtime and its stored row hold the same text, so asserting on one proved nothing about which was used. The test now continues the session with a different agent object, which separates them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From adversarial review of the previous commit. The test named for resume never called resume. It rebuilt the prompt a second time — the same call the next-turn test already makes — so the criterion it was flipped green on was asserted by the test's name, not by its body. Resume has its own reload, its own runtime call and its own persistence; none of that was exercised. It now pauses a turn on a confirmation tool, edits the agent's instructions, resumes with an approval, and asserts the resumed turn was sent the current text, that the approval still gated the write, and that the stored transcript is neither rewritten nor duplicated. The record lookup in the instructions helper was unreachable. Both callers of the prompt builder dereference the runtime unguarded in the same call, and sessions only come from new_session or load_session, which always attach one. The branch could never run, and had it run it would have diverged: a raw field read, bypassing the permission and disabled-agent checks that assembling an agent performs. Removed. The blank-instructions fallback was documented but untested, and the docstring overstated it in two ways: it said the instructions come from the record when they come from the runtime the session was loaded with, and it implied a fallback exists even for a session with no stored row to fall back to. Both corrected, and the fallback now has a test. One weak test is kept rather than deleted, relabelled to say what it is: a code session's runtime and its stored row hold the same text, so it cannot discriminate. The test that can is the one after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Someone approving a write was shown the tool's internal slug and a raw dump of
the model's arguments. A plain-English summary was possible only for tools
written in Python, because it was a callable attribute; every tool defined as a
record — which is how the product's own tools are built — fell back to the dump.
That is the worst possible moment for a comprehension gap.
A tool record can now carry an Approval Question: a short template, filled in
from that call's arguments. The runtime tool also carries the record's title, so
the first line names the tool the way a person would.
The code callable still wins where a tool has one. The arguments are still shown
where neither exists, and whenever a template cannot be filled in — it raises,
it names something the call did not supply, or it comes out empty. Rendering can
never take the question away: a badly written template must not be able to stop
a person being asked.
Nothing about the decision moves. The options are still exactly Approve and
Deny, free text still redirects, and only the exact Approve still executes. The
only lines removed from the agent are a docstring line, an else, and the prompt
string itself.
One thing worth knowing for anyone touching this: asking the renderer not to
treat the template as a path is not enough. Its check is "is_path or looks like
a path", so the guess still runs, and it fires on any single-line string whose
last dotted segment resembles a file extension — which an ordinary question does:
"Delete the report for {{ customer }}.txt". Such a template silently fell back,
and a crafted one could have pulled a template file off the disk. It only guesses
when the string has no line break, so it is given one, and the result trimmed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ut it
From adversarial review. Three faults, one root: the sentence a person reads
before authorising a write was partly written by the model.
A value could push the question out of sight. The body was cut at a character
limit with no marker, so an author's "Update {{ customer }} and DELETE every
invoice they have" plus a padded customer name left a question that still read
as whole and no longer mentioned the delete. The arguments dump it replaced had
no such limit, so this was worse than what it improved on. Each value is capped
instead, and a question whose own text is too long is refused rather than cut.
A value could forge a second question. Interpolated as prose, a value could open
a line of its own and write a reassuring one underneath the real one. The dump
escaped newlines; this did not. Values are flattened to a single line now.
The question could read the database. It rendered in an environment carrying
the platform's template globals — enough to query ignoring permissions, or to
reach the network — and it rendered as soon as the model proposed the call,
before anyone approved it and even if they refused. The globals are gone from
that environment and a question may name only the call's own arguments.
Three smaller things from the same pass. Reading the new field off the document
raised on a site that had not migrated yet, taking out every tool rather than
the templated ones. A failed render queued a server traceback for the browser,
naming the platform to the user at the moment of approval. And a question
guarding a declared-but-omitted optional argument was refused, which is the
failure this feature exists to remove.
Tests 22 to 31. Review showed fourteen of the first twenty-two survived deleting
the feature outright, because the safety tests asserted only that the fallback
appeared — which is also what happens with no feature at all. Each refusal now
carries a positive control in the identical form, and the checks that could not
discriminate assert the unit directly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by adversarial review of the previous commit. The room left for an attachment was measured from the stored rows. That used to be honest, because the stored rows were what the model received. After this branch they are not: the system message is rebuilt from the agent record and can be far longer than the row it stands in for, and the difference was invisible to the budget. Measured on the bench: with two hundred thousand characters of instructions the budget still offered 495,604 characters while the prompt already held 200,202 of a 512,000 window — about fifty thousand tokens of room that does not exist. A long set of instructions was silently buying itself space an attachment would then overrun. The budget now counts the difference between the instructions being sent and the stored row they replace, which is zero for a session driven from code, where the stored rows really are what goes. Two tests: one that the budget gives up the hundred thousand characters, and one for the invariant that matters — what is sent plus what is still on offer has to fit the window. Also records three things the same pass surfaced that are not fixed here: only the system message at index 0 is replaced, so a stored system row at a later index would still be replayed (the engine cannot produce that shape); a record-linked session continued with a code agent runs that agent's text; and the ordering test is a real contract test but does not on its own prove this change, so the ship table no longer claims it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The question a person is shown before a write is filled in from the call's own
arguments by a single regex over {argument_name}. There is no expression, filter,
attribute access, index, loop or conditional, and no template engine: an engine
that can evaluate is an engine a caller can steer, and what it steers is what
someone sees at the moment they authorise a write.
Values are quoted and shown whole or not at all. Control and format characters,
bidi overrides included, are escaped rather than obeyed, so a value cannot open
a line of its own and write a friendlier question underneath the real one.
Nothing is ever shortened: a value over the cap, a missing argument or a value
that is not a single value abandons the sentence and shows the arguments alone,
because a question showing part of a value reads exactly like one showing all
of it.
The exact arguments that will execute are shown beneath the sentence every time,
so an administrator's wording is never the only thing a person sees.
Replaces the templating introduced in 3145d93 and hardened in 4f32183. Those
commits are kept: the three HIGH issues found against them are each a named test
here.
Two reviewers, independently, got the forged second question back by
substituting U+2028 for the newline. LINE SEPARATOR and PARAGRAPH SEPARATOR
are categories Zl and Zp, and the escape rule named Cc and Cf — so a value
could still open a line of its own, in an approval question, which is the one
place that must not happen. The rule is a whitelist now: everything in a C* or
Z* category is escaped, the ordinary space excepted, so unassigned code points,
private use and lone surrogates are covered and a later revision of Unicode
cannot add a new way through.
Also from review, each with a test that goes red without it: the total length
cap and the above-the-BMP escape had no test; the guard that makes rendering
unable to raise had a test that passed with the guard deleted; and the title
was escaped with nothing proving it. An over-long value is now refused before
it is escaped character by character, the arguments are serialised only where
they are used, and the body is joined to the first line rather than passed to
a formatter, so a value shaped like {0} has nothing to be read as.
A pause can carry more than one question, and they are answered together as one
group. Answering {"k1": "Approve", "k2": "Deny"} ran k1's tool and only then
halted the turn, because _prepare_resume resolved every pending call before the
caller consulted _has_denial. A person who refused one of two actions shown
together had not stopped the other one.
_prepare_resume now reads _has_denial once, before it resolves anything. If the
group holds a denial, an answer of exactly "Approve" resolves to an
approved-but-withheld record instead of running its tool. The run halts exactly
as it did, every answer is still recorded against its own call, and the model is
told why nothing ran.
Nothing else changes. Only the exact "Approve" ever executes; the denied and
redirect records, the options, allow_other and single-question behaviour are all
byte-identical, and _invoke, _resolve_confirmation, _confirmation_question and
_has_denial are untouched.
18 tests: six for the new behaviour, twelve pinning what must not change,
including a source comparison of the four functions above against their
unmodified form.
…public path The pin that asserts the four confirmation functions are unchanged resolved its baseline by shelling out to git and skipped itself when that failed. It passes on a full clone and silently no-ops anywhere the comparison branch is not a local ref, which includes CI: the workflow checks out a single ref. A gate whose failure mode has never been observed is a comment, so it now carries the four functions' digests as literals and cannot opt out. Two controls come with it: one proves the comparison can report a difference, one proves the file being hashed is the module the tests ran against. Adds a test that drives the whole stack rather than the agent alone: the whitelisted resume endpoint, a record-backed session, and the stored transcript read back. Every other test in this module calls resume directly, so nothing would have caught a reshaping of the answers map or the prompt messages on the way in.
Five scenarios covering the approval path: a read-only tool that never pauses, a write approved with the exact "Approve", a denial that executes nothing and does not consult the model again, free text that redirects without executing, and two questions in one pause where a denial beside an approval must execute neither. Nothing outside evals/ is touched. The runner is plain Python, builds a code agent around a scripted model, and never constructs the platform's own model class. It refuses to start if the environment carries any provider credential, which is a hard error rather than a warning, and twelve tests cover that refusal including a positive control. The wrapper derives its verdict from the runner's summary line rather than from an exit code, and treats a missing summary as a failure. Four of the five pass here. deny_stops_the_batch asserts the behaviour S14 introduces and is red until that merges; it reproduced the defect independently on its first run, and is left red rather than weakened.
…ng it The runtime that resumes a run is rebuilt from the record and is not always the one that paused it. A tool can be removed, renamed or disabled while a run waits for an answer, and a tool that required confirmation when the question was asked may not require it any more. Both fell into the branch written for a question a tool asked itself, where the answer is serialised as that call's result. So a person who read an approval question and answered "Approve" had the string "Approve" written in as the tool's own output: nothing executed, nothing was denied, the run completed, and the model could reasonably report the action done. _prepare_resume now fails closed in both cases. A pending call whose tool is not in the runtime, and one whose tool is no longer gated although the person was asked to approve it, are recorded as not executed with a fixed message that says nothing was done — never with the answer. What was asked is read from the questions the pause recorded, since the runtime is the thing that changed; with no such record the behaviour is exactly what it was. The exact-"Approve" path, the ["Approve", "Deny"] options and the group rule are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…asked The first commit closed the direction where an approval is dropped. Review found the mirror, and it is the direction that executes. A tool that asks its own question is not asking for approval. If that tool is given a confirmation requirement while the run is paused, then at resume it is present and gated, so it looks exactly like a call a person approved — and an answer that happens to read "Approve" runs it. Nobody was ever shown an approval question for that action. So the test is stated once, for both directions: what the person was asked and what the tool now requires must agree, or nothing runs. With no record of the pause the tool's own requirement stands in for what was asked, so the two cannot disagree and the behaviour is unchanged. Also: the options a question is recognised by are now immutable, a bare string in an eval's text assertion is read as one string rather than a sequence of characters, and three tests that asserted their own copies of a value now assert the engine's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ls closed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It is a harness artefact of the session that happened to be running, not part of S15. The file itself is untouched on disk; only the index entry is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t for the ones still open Four scenarios join the five already here: a tool gone by the time an approval arrives, an ungated call running in the same turn as a gated one before anyone is asked, two tool calls sharing one id so a single approval executes twice, and a pause answered by a runtime that did not create it. Three of those describe defects that reproduce today and that no spec has claimed. They assert the behaviour that should happen, so they fail — and a suite where that is indistinguishable from a regression is a suite nobody reads. So a scenario may now carry a `known_defect` key saying what the defect is and where it is written down, and the runner reports four outcomes rather than two. A known defect that fails is reported as such, with its reason, on every run, and does not hold the gate red: it is already written down. A known defect that PASSES does hold the gate red, because the code was fixed and the record was not, and the suite is then asserting something untrue about the system. That is the only outcome whose fix is to edit a scenario rather than the engine. A bare `known_defect: true` is refused. One that does not say what it is cannot be told apart from a scenario somebody silenced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ine rule and the free-text answer
A7 and A9's third token were the two acceptance rows with no render proof. A7 is asserted in both
directions in one test — the engine's own first line is the headline for its approval pair, the
client's computed label otherwise — because either half alone is satisfied by a card that always
shows one of them. A9's Approve and Deny were covered; "Other" was not, and it is the only answer
whose content comes from the person.
Each is reddened by the mutation the spec itself nominates for that row. The Other test also
catches a one-word change that makes a free-text redirect emit the approval token.
The headline assertion stubs the desk's global translator for its own length: lib/translate.js
returns a message unsubstituted when that global is absent, so a bare jsdom renders the literal
"Delete 1 {0} record" and the test would otherwise be asserting the fallback instead of the card.
Run 13 built 25 render tests for the approval card and proved they go red on demand, but GATE=GREEN said nothing about frontend/ because hard limit 5 forbids an agent editing scripts/. Probed: a deliberate failure gives GATE=RED reason=js-suite-failed, and the file restores byte-identical.
…roval card
S21 escaped the approval question's body. The arguments table beside it was never
escaped, and Vue's mustache escapes HTML and nothing else — so a model-chosen value
carrying a newline drew a real second line inside the card, above two buttons
labelled Approve and Deny, and a bidi override reordered the sentence around it
while adding no character. A value of "x\nApproved by admin" forged a line of
authority on the one surface whose whole job is to be trustworthy.
Measured by rendering, not by reading source: ten paths reach the DOM on this
surface and nine of them were affected — CodeBlock's pre-wrap <pre> (twice: the
block row and the unparseable payload), the two-column cell, the tuple inside a
filter condition, ChipList, RecordsList's title, the nested recursion, the argument
LABELS (the keys are the model's too), and the headline the client composes from
`execute`'s own description.
One rule, in one module. `lib/display.js` is a faithful port of the engine's
`_escaped`/`_for_display` — the same C*/Z* whitelist, the same escape-then-cap
order, the same unquoted elision count outside a quote no value can write. The
engine's helper is Python-side only and the panel renders values out of its own
store, so there is nothing to call; a port in one place is the nearest thing to one
escaper. Text is shown as ITSELF when escaping would not change it and it fits, so
an ordinary card is unchanged and the appearance of a quote is itself the signal.
The rule that keeps the legitimate case: a value renders as raw, multi-line text
when — and only when — the tool's own declaration says that argument is code
(`CODE_ARG_KEYS`, keyed by tool and argument name, in reviewed client code). The
SHAPE of a value never earns it that right. `execute`'s code keeps its real lines;
a second multi-line argument on the same call does not.
Also fixes the fallback in `lib/translate.js`, which returned every positional
message unsubstituted: away from the desk the headline read `Run "{0}" on {1}` and
named no action, which both broke the card and hid the headline's model-chosen
values from every test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ith the mutation it names
Run 13's reviewer applied ten mutations and watched 25/25 stay green. It fixed none of
them: its report arrived eighteen minutes before the cap, and shipping a fix whose
mutation was never watched go red is the failure this project keeps recording. So each
finding here carries its own probe, and every one of the seven was watched red before
this commit existed.
HIGH 1 — a height cap on an ANCESTOR, written as a scoped CSS rule on a non-Tailwind
class, was invisible to a walk that read `el.className` and `el.style` only. The new walk
reads `getComputedStyle` on every element up to `#flow-root`, with the component's own
compiled declarations as the positive control that "" means "no such rule" and not "no
stylesheet at all".
HIGH 2 — the no-cap claim was synchronous, so every clamp-after-layout shape (`nextTick`,
`rAF`, `ResizeObserver`, a watcher) was green by construction. The claim is now made
before AND after the card has settled.
HIGH 3 — the headline is model-controlled (`confirmTitle` carries `execute`'s own
`description`) and every markup assertion was scoped to `pre.flow-confirm-body`, so
`v-html` on the headline stayed green. One text node, no elements, entities in the markup.
MEDIUM 6 — the headline's own clamping, which `textContent` cannot see and the body's
ancestor walk never visited.
MEDIUM 7 — `window.__` is undefined in jsdom and the fallback is the identity, so
`emit("answer", __(opt))` was indistinguishable from `emit("answer", opt)`. A marker
translator makes the label and the token two different strings, and the approval path
rests on the token.
MEDIUM 8 — the specs hand-built `{ prompt, options }` and dropped every other field the
engine sets. The card BRANCHES on one of them: `allow_other !== false` is what draws the
free-text affordance two tests prove exists. `engineQuestion()` carries the fixture's
question in by spread, so a field added later arrives without anyone remembering, and the
fixture guard now captures `allow_other` for every case — an engine-side flip reddens
those two tests instead of hiding inside them.
MEDIUM 9 — production has two argument shapes this suite never mounted: a JSON STRING
(what every reloaded session delivers, and where `JSON.parse` turns a stored ``
back into a real override before anything renders it) and no arguments at all (the first
frame of every card, since `tool_started` fires twice).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…question with no prompt still draws a card
Both found by the reviewer over the final diff, both watched red before being fixed.
The context line. `toolContext` returns the first of `doctype`, `search` or `action`.
`action` went through `humanize` and so was escaped by this branch; `doctype` and
`search` were returned as the model wrote them and rendered as `· {{ context }}` in the
timeline the approval card sits in. That is the line a reader scans to see WHICH doctype a
call touches, and a right-to-left override there reorders its visible text while adding no
character — the same attack the card itself was just hardened against, one component over.
The claim this branch makes is "one rule, in one module, called by every path that turns a
model value into text"; this path did not call it, and now does.
The missing prompt. Reading the headline off the engine's own first line made
`paragraphs` unconditional, so `prompt.split` is now reached on every card rather than only
on the free-text branch. `store.js` spreads a stored question row verbatim when a paused run
is resumed and validates nothing, so a row without a `prompt` — an older schema, a partial
write — threw a TypeError inside a computed during render: not a degraded card but no card,
an approval that can no longer be answered. There were two unguarded dereferences, the
second found only because the test for the first asserted that the buttons are still
there. Both now read one guarded value: a missing question is an empty one, not a crash.
Also removes a false comment from `display.js`. It claimed `displayText` is idempotent on
text the engine had already escaped. It is not, by construction — the escaper maps `\` to
`\\`, so engine-escaped text is escaped a second time and quoted. No caller relies on it
today; the comment is the kind a future patch would cite to route the question's own
sentence through a second escaper, so it says the true property instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not by an expression's name
Run 12 pinned "when the headline is not the engine's own first line, the body is the WHOLE
prompt" with `assertIn("props.question.prompt.trim()", body)` — the property stated as a
literal source string. The guard the final reviewer asked for renames that expression to
`promptText.value.trim()`, so the gate went RED on a test whose property the guard keeps
intact: the body still renders the whole prompt, and the reviewer's own snapshot of twelve
ordinary surfaces — the one-paragraph card among them — is byte-identical across the fix.
The assertion is not relaxed to make the gate green. It is replaced by a strictly stronger
one: the renamed expression AND the guard's own shape AND the absence of any direct
dereference of the stored value anywhere in the card, with a positive control proving that
last detector fires on the shape it forbids. The old string could be satisfied by the
unguarded read the reviewer found; the new assertions cannot.
Both probed, each restored from a byte backup and proved by sha256:
- run 12's own QA mutation (`titleIsEngineHead.value ? "" : promptText.value.trim()`, which
blanks the body on exactly the live approval path) -> GATE=RED, the `? ""` assertion and
the JS suite both firing.
- the guard deleted (`promptText` reads the stored value directly) -> GATE=RED on the new
source assertion and on the JS test for a non-string prompt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…whole card
The tool NAME is as model-chosen as the arguments beside it, and run 14's escaping never asked
anything of it. Two plain object literals were indexed with it, so eight names the model can pick
resolved to a member of `Object.prototype` instead of missing:
`CODE_ARG_KEYS["__proto__"]` returned `Object.prototype` — truthy, so it was returned in place of
the empty Set — and the one thing every caller then does is `codeKeys.has(key)`. That threw
`has is not a function` inside a computed during render, on the approval card and in the activity
log. Not a degraded card: NO card, and so an approval nobody can answer, reachable by a model that
simply names its tool `__proto__`.
`LABELS["constructor"]` returned the `Object` function, so the headline read
`function Object() { [native code] }` and `humanize(name)` — the only branch that shows the real
name, escaped — was never reached. The card named something that was not what would run.
Both become Maps. A Map has no inherited keys, so a miss is a miss for every string: a model-chosen
key is DATA, and data is looked up in a structure holding only what was put in it. `blockKeysFor`
still returns a fresh Set on a miss, as before.
Then the property, in place of five more examples. `displayProperty.spec.js` is table-driven over 37
hostile classes — bidi overrides AND the isolates that are a different Unicode block, the zero-width
set, the Cf category generally including an astral member, Cc, Zs/Zl/Zp, a lone surrogate, Co, Cn,
and the two boundary characters — and asserts one thing: the text a person reads in the card, run
back through an inverse of the declared normalisation written inside the test, is the value that will
execute, character for character. Plus the half a string comparison misses, that no two values READ
alike, with the control that the same comparison collapses them under a newline-only escaper.
And A3's answer, asserted rather than asserted-about: a markdown renderer exists and is a real
v-html sink, but the card is on the sibling branch of the one `v-for` that reaches it, so aggressive
markdown in the question and in an argument produces zero markup inside the box — with the identical
strings, through the component that DOES render markdown, as the positive control.
GATE=GREEN TESTS_RUN=1076 FAILURE_LINES=0 JS_TESTS=174 JS_EXIT=0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the next run must finish or revert this before starting anything new
…dy can price Task 0 of this run's second sitting. `366bed8` parked Part B when the tmux server died and its own message said the next run must finish or revert it. FINISH: its content is not a broken half-edit — B1 and B2 are written, commented and tested — and it is gate-green as committed, measured here before anything was added to it (TESTS_RUN=1088 FAILURE_LINES=0 JS_TESTS=176 JS_EXIT=0). Reverting correct work to re-derive it would have spent the whole of this sitting's budget. No history is rewritten; this commit completes the one item `366bed8` never reached. B3 — `cached_prompt_tokens` — DECIDED: DROPPED, and the drop is now load-bearing rather than accidental. Grep for the name across every .py/.js/.vue/.json outside node_modules returns nothing, with `prompt_tokens` in the identical form returning twenty hits as the positive control: the count is not captured here today, because the code that would capture it was a proposal and was never built. Keeping it would need somewhere to price it against, and that is `cached_input_per_1k` on a rate doctype in another team's read-only repository, which has seven fields and no cached rate among them. So capturing it in this repository puts a number in the record that nothing can turn into money — the one option the review says is unavailable — and it would falsify, from the outside, a shipped caveat that says an estimate cannot see cached input and that is not in this repository to amend. What makes the drop worth a test rather than a shrug is B1. `_accumulate_usage` now sums EVERY key it is handed, so one line in `_normalize` would carry a cached count all the way to the stored record with no further edit. `TestNoCountIsCapturedThatNothingCanUse` pins the emitted key set as a whitelist — the three counts, plus `served_model_mismatch` and nothing else — so that line cannot land quietly, and it names the cached spellings explicitly so a failure says which decision was reversed rather than merely that a set changed. PROBED, both paths, because the first mutation left one of them green: `_normalize` + cached key -> RED, 3 failures (streamed test stayed GREEN — different path) `_consume_stream` + cached -> RED, 1 failure (the streamed test, alone) flow/lib/model.py sha256 8bcb324f00264b9bf7b70299a2d362c045bc478a3dd2e2078ebf1ad84dba0f4f before each mutation and restored to the same hash after each, by byte backup, `git diff` empty. The platform-side change that would reverse this decision is recorded in the test's docstring, so reversing it is a known piece of work and not a discovery. GATE=GREEN TESTS_RUN=1092 FAILURE_LINES=0 JS_TESTS=176 JS_EXIT=0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a counter that always fired
Part C put one fresh adversary over `veyqon..HEAD` with a mandate to mutate every finding. It
returned two HIGHs, both confirmed here independently before anything was changed, and both real.
HIGH 2 — A CHARACTER CLASS THE WHOLE RUN CLAIMED TO HAVE CLOSED AND HAD NOT.
The rule on both sides was "escape everything in a C* or Z* category". Unicode has a property that
says exactly what these escapers are for — `Default_Ignorable_Code_Point`, "a conformant renderer
paints nothing here" — and **267 of those code points are in neither C* nor Z***. Four are category
`Lo`: LETTERS. U+115F, U+1160, U+3164, U+FFA0. No rule about controls, formats, separators or even
combining marks was ever going to reach a letter.
So they arrived RAW AND UNQUOTED — without even the quote that this design makes the signal that
something was escaped, because a value the rule finds nothing wrong with is shown as itself. `SO-0001`
and `SO-0001<U+3164>` are two different writes that render identically, on the approval card, in the
activity log, and in the engine-composed sentence a person actually approves. That is A2's attack
whole, on a surface this run had already reported as closed. The reviewer found it by measuring the
property against the rule; reading the rule would never have shown it.
Both escapers now take the property as a third term. `display.js` adds
`\p{Default_Ignorable_Code_Point}` to the one regex; `agent.py` cannot — `unicodedata` does not
expose the property — so `_paints_nothing` carries the 17 ranges, derived from the same property the
panel matches and **measured against it across all 0x110000 code points: 0 divergence on the new
class**. (The 4803 that still differ are the pre-existing Unicode-version skew between the two
runtimes, in the safe direction, and are recorded OPEN, not introduced here.) It takes in the
variation selectors too, which is a decision and not an oversight: an emoji written with U+FE0F now
renders quoted. Slightly uglier, and correct — the other choice is a class of invisible character
judged safe by whoever last thought about it.
RED FIRST: 9 rows added to `HOSTILE` and 4 values to the injectivity list ->
`"de<U+1160>lete" reads the same as "delete"`, 10 of 98 JS tests RED, the forgery demonstrated
rather than described. Python: 9 failures. `asRead` strips default-ignorables now, or the
injectivity test cannot see the collapse it exists to catch.
HIGH 1 — `served_model_mismatch` FIRED ON 100% OF CALLS, and no fixture could see it.
`asked_for` is `Model.model_id`, and `Flow Model` VALIDATES it into `provider/model` form. litellm
strips that prefix before the request leaves, so the reply's identifier is the bare name and never
equalled what was asked for. Every turn unpriceable; the stored count a duplicate of the model-call
count. Every fixture in the module was an invented bare name with no prefix, so the suite pinned
neither the decision nor its inverse. Verified against the real library, not the fixtures:
`litellm.completion(model="openai/gpt-4o-mini", ...)` returns `.model == "gpt-4o-mini"`.
The prefix is routing syntax — which adapter places the call — not identity, so it comes off first
and nothing past it is normalised. `gpt-4o-mini` no longer counts; `gpt-4o-mini-2024-07-18` and
`gpt-4o` still do. `rsplit`, because a model part may hold slashes of its own. RED FIRST: 2 failures.
MEDIUM 3 — THE TEST THAT COULD NOT GO RED. `answersAreNotDropped.spec.js` was a grep of `store.js`'s
source text. The reviewer left the safe line in place, added a plain object beside it, sent that one,
and all 176 tests stayed green with the defect fully restored — an approval a person clicked again
transmitted as `{}`. The three lines now live in `@/lib/answers` so they can be CALLED; every
assertion is on a returned payload. Written with the defect in place first: 2 of 4 RED.
MEDIUM 4 — the accumulator floored a per-call cost of 0.004 to ZERO on every call, silently, while
its own docstring offered "a cost in micros" as the kind of key it now sums; and `int(float("inf"))`
raises `OverflowError`, which the `except (TypeError, ValueError)` did not catch, so a turn that had
done its work died over its own meter. Fractions stay fractions, integers stay integers, non-finite
is skipped. RED FIRST: 1 failure and 1 error, exactly the two shapes.
Still OPEN, deliberately: the Unicode-version skew (LOW 5); `served_model_mismatch` reaches no screen
and no pricing code (LOW 6 — the resolution half lives in another repository); strong-RTL reordering,
which needs CSS isolation and a real browser, not an escape; confusables; U+2800.
GATE=GREEN TESTS_RUN=1103 FAILURE_LINES=0 JS_TESTS=196 JS_EXIT=0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssertion that was blind
The reviewer was re-run over `c9c32c9` and confirmed both HIGHs CLOSED by independent measurement,
not by reading the claim: `_paints_nothing` matches `\p{Default_Ignorable_Code_Point}` exactly (4174
each way, 0 in either difference), a four-point boundary probe at all 17 ranges found no off-by-one,
a `Bidi_Class=BN` sweep — Unicode's own "removed before rendering" class — returned 0 unescaped, and
escaper parity is unchanged at 4803 Python-only / 0 JS-only, so the new term added no drift. No
CRITICAL and no HIGH remained. What it found instead were four things worth closing, one of which I
caused.
MEDIUM 1 — I UNPINNED THE CALL SITE WHILE FIXING THE TEST. Moving the payload builder into
`@/lib/answers` made the function testable and deleted the only assertion `store.js` had. The
reviewer re-ran its original mutation against the new arrangement — `answers.js` untouched and
correct, a plain object inlined beside the call — and got 196/196 green with the defect fully
restored. `store.js` was LESS pinned after the fix than before it. The old grep at least refused
`const answers = {};`. A grep for the wiring is back, and it is the honest kind: it proves the only
proposition left over, that the module a person's decision travels through is the one the four
behavioural tests are about. RED FIRST, with the mutation still in the tree: 1 failed | 102 passed.
LOW 4 — AN ASSERTION THAT COULD NOT SEE THE CLASS IT WAS UPDATED FOR. `displayProperty.spec.js`
widened `asRead` to three terms and left `expect(/[\p{C}\p{Z}]/u.test(...)).toBe(false)` at two,
under a comment claiming nothing invisible reaches the card. Measured: with the DI term deleted from
`display.js`, that assertion passed for all nine new rows — only the round trip and the injectivity
test went red. An assertion about "nothing invisible" has to use the same definition of invisible as
the rule it checks. Probed after widening: all nine now fail under the same mutation.
MEDIUM 2 — THE SAME DEFECT ONE TYPE OVER. The float fix branched on `int` then `float`, so
`Decimal("0.004")` took the `int()` fall-through and floored to 0 on every call — and `Decimal` is
what the framework's Currency reads hand back and the obvious type for the cost key the docstring
invites. The first repair here was wrong too and the test caught it: `Decimal` is registered against
`numbers.Number` and DELIBERATELY NOT against `numbers.Real`, so a `Real` test still floored it.
`numbers.Number` minus `complex` is the predicate. The exception list went to `except Exception` —
it was `(TypeError, ValueError)`, then `(TypeError, ValueError, OverflowError)`, and a foreign
`__int__` raising anything else still killed the turn. A count is never worth a turn. RED FIRST:
1 failure and 1 error.
MEDIUM 3 — TWO CLAIMS IN MY OWN DOCSTRING WERE FALSE, and both are corrected rather than softened.
"The prefix is routing syntax, not identity" is wrong: a configured fallback changes the PROVIDER,
which is the case the counter's own docstring uses to motivate it, and for pricing the adapter IS
part of the identity. A call written `openai/gpt-4o` and answered from Azure records nothing. The
narrowing stays — the serving provider is only on a private `_hidden_params` attribute of a
third-party library, and coupling the engine to that deserves its own spec, not a line smuggled into
a bug fix — but it is now stated as OPEN in the function, with the exact fix. "`Flow Model` refuses
anything else" is also wrong: a `Model(model_id=...)` built directly, the code-only Agent path, never
passes the validator. Both corrected in place.
Also from that finding: `MODEL_ID_PATTERN` permits a trailing slash, `rsplit` then yields the empty
string, and such a model counted on every single call — the always-fires bug surviving inside a
corner of its own fix. `rstrip("/")` first. RED FIRST: 1 failure.
Recorded OPEN rather than closed quietly: the twelve VISIBLE-WIDTH blank characters the reviewer
swept out (U+2800 was one example of a class, and two of the twelve are `Lo` letters); the Unicode
version skew; the provider dimension above; strong-RTL reordering, which needs CSS isolation and a
real browser; confusables. And one cost of the widened escape, measured: an argument of 29 emoji
written with U+FE0F now exceeds `CONFIRM_VALUE_LIMIT` once escaped, so the plain-English confirm
sentence is dropped in favour of the raw JSON dump — a degradation of a surface, not a hole, since
the approver still sees the whole value.
GATE=GREEN TESTS_RUN=1107 FAILURE_LINES=0 JS_TESTS=197 JS_EXIT=0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
U+200D ZERO WIDTH JOINER carries Unicode's Default_Ignorable_Code_Point
property, so run 15's invisible-character work marked it — correctly, for
every script that does not use it. In Sinhala, Tamil, Devanagari and every
other Indic script a joiner is SPELLING: a conjunct is written CONSONANT +
VIRAMA + ZWJ + CONSONANT, so ශ්රී ලංකා reached the approval card as
ශ්රී ලංකා — six characters of machine escape dropped into the middle
of the first word of this company's own country. The defence that exists so
a person can read what they are approving was making the text unreadable for
the people who read it.
The rule is PORTED, not invented: two sibling front ends decided it first and
tested it (`joinsRatherThanHides`, "a joiner that FOLLOWS a virama is
orthography, not hiding, and is kept"). It is then narrowed in two ways they
are not, and each is a hole they still carry:
1. A LETTER MUST FOLLOW. Their version never looks past the joiner, so a
joiner at the end of a value, before a space, before punctuation or
doubled keeps its exemption while joining nothing.
2. THE TWO NEIGHBOURS MUST SHARE A 128-CODE-POINT BLOCK. Without it
SO-000A + DEVANAGARI VIRAMA + ZWJ + x satisfies every other clause while
the joiner forms a conjunct with nothing, so it is purely hidden.
And the virama set is READ FROM THE LIBRARY — `unicodedata.combining(ch) == 9`
is Unicode's own Virama combining class — rather than written out. The front
ends write sixteen by hand; measured on UCD 16.0.0 there are sixty-nine.
Display only. `TestTheValueThatRUNSIsUntouched` approves a real gated call and
compares the bytes the tool body received with the bytes the model produced,
with a positive control whose card and wire deliberately disagree.
Each of the four clauses was mutated and watched go red. The third one was
GREEN under its first mutation — clause 4 was masking it — so four more tests
isolate it (a joiner before a vowel sign, a second virama, a digit and
punctuation of the SAME script, all of which clause 4 admits).
Recorded and NOT fixed: `_confirmation_question`'s untemplated fallback body is
`json.dumps(..., ensure_ascii=True)` and never passes through `_escaped` at
all, so on a card with no sentence EVERY non-ASCII character is escaped, not
just the joiner. It is a rule-4 function and this run's brief does not name it,
and the one-word change is unsafe (`ensure_ascii=False` alone would put a
right-to-left override on the card raw). Two tests pin the present behaviour.
GATE=GREEN EXIT=0 TESTS_RUN=1139 FAILURE_LINES=0 JS_TESTS=197 JS_EXIT=0
`frontend/src/lib/display.js` is the panel's own copy of the engine's escaping rule — it says so in its header, and says why: the arguments beside the approval question are shipped as data and rendered by the panel, so there is nothing for it to call. The copy carried the same defect, so the fix is ported the same way, clause for clause, from `_spells_rather_than_hides`. The one difference, and it is forced: JavaScript's regular expressions cannot ask for a canonical combining class, so where the engine reads the viramas out of `unicodedata`, this copy carries a table of sixty-nine code points. The table is GENERATED and never typed, and the next commit adds the test that recomputes it from Python on every run and fails if one entry differs. Five mutations, five reds: each of the four clauses deleted in turn, and one code point of the virama table changed from U+0DCA to U+0DCB. GATE=GREEN EXIT=0 TESTS_RUN=1139 FAILURE_LINES=0 JS_TESTS=232 JS_EXIT=0
…tch it Four copies of the same 17 ranges is why three of the four were wrong. Nothing can be shared between the repositories, so the remaining defence is that the rule is written down ONCE with the reason for every clause, and that each copy is measured against the writing. `brain/40-architecture/the-invisible-character-rule.md` is that writing: where the copies are (there are FIVE, not four — the consolidation missed the Desk panel), the three terms and why each was added after the previous one proved insufficient, all 17 ranges with what is in them, the four clauses of the joiner exception with the shape each one refuses, all 69 viramas, 21 test vectors, what is deliberately NOT defended, and the paragraph `q_flow_tools` needs. `flow/tests/test_s24_one_rule_four_copies.py` PARSES that note and fails when a copy in this repository drifts from it — the ranges against the engine's table, the viramas against `unicodedata` AND against the panel's generated table, and every vector against `_escaped`. The same vectors run against the panel from `joinerInsideAWord.spec.js`, so a divergence fails on both sides. Seven mutations, seven reds: a range changed in the note, a virama dropped from the note, a vector's expected output changed, the stated total changed, a range changed in the ENGINE, a code point changed in the PANEL's table, and the sentence naming the orthography removed. Every restore is a byte copy proved by sha256. Also asserted here because nothing else did: `_DEFAULT_IGNORABLE_RANGES` is sorted and disjoint — `_paints_nothing`'s early exit is only correct if it is — and the scan agrees with plain membership on both edges of all 17 ranges. GATE=GREEN EXIT=0 TESTS_RUN=1154 FAILURE_LINES=0 JS_TESTS=232 JS_EXIT=0
…a rule too wide **F1 — the fix never reached the cards it was for.** `flow/tools/builtins.py` `_for_display` and the panel's `quoteForDisplay` escape and cap in one pass, and both did it by handing the escaper ONE CHARACTER at a time. A one-character string has no character either side of it, so clause 1 refused every joiner and the exception was STRUCTURALLY UNREACHABLE — on the path the six shipped write builtins use, which is where production write confirmations come from. The escaper was fixed and the card a person reads was not. The decision is now exposed as `escape_for_display_at` / `escapeCodePointAt`, taking the index into the whole value; the caller chooses where to CUT and never what a character means. **F2 — combining class 9 is not "this script spells with a joiner".** It says how a mark REORDERS. Thai U+0E3A PHINTHU, Lao U+0EBA, Tifinagh U+2D7F, Myanmar U+103A ASAT and Brahmi U+1107F NUMBER JOINER all carry it and none takes a joiner, so `กฺข` and `กฺข` were two different writes painting the same pixels, raw and UNQUOTED. The exception is now confined to U+0900..U+0DFF, the ten Brahmi-derived blocks whose joiner orthography the standard documents. Twelve viramas, not sixty-nine. **F3 — clause 4 had been measured in the wrong direction.** The note measured which conjuncts the block test MISSES (false negatives, harmless — they escape). Nobody measured the false positives: 27 viramas have a letter of a DIFFERENT script inside their own block, so U+2D7F TIFINAGH licensed U+2D00 GEORGIAN and U+10A3F KHAROSHTHI licensed U+10A60 OLD SOUTH ARABIAN — the exact cross-script shape clause 4 existed to refuse. Inside the new range there are ZERO such pairs, so `cp >> 7` IS the script there rather than a proxy. Asserted over the whole of Unicode, not over examples. **F4 — the tests that existed to prove the defence was not dissolved could not go red.** They asserted STRING inequality, and the escaper never deletes a code point, so string injectivity is unconditional: under a mutation that kept every joiner everywhere, `keeps "paid" and "paid<ZWJ>" apart` stayed green at the moment those two displayed identically. They now assert the pairs differ in characters a renderer PAINTS, with the conjunct pairs — where invisibility is the declared cost — named one by one in a second list. F6: the JS vector table had been hand-copied and held 8 of the note's vectors; it is generated from the note now, and `test_s24` fails if the two differ by a line. F7: both copies pointed at a spec file that never existed. F5 is pinned, not closed: Python UCD 16.0.0 and Node Unicode 17.0 disagree on 98 neighbours, all of them unassigned-in-16 letters-in-17, and the panel is the permissive side. Closing it needs a fifth generated table and its own decision. Four probes, four reds: builtins back to per-character; the range check removed; the whole exception widened to `return True` (which the F4 tests now catch); and one vector dropped from the JS table. GATE=GREEN EXIT=0 TESTS_RUN=1170 FAILURE_LINES=0 JS_TESTS=259 JS_EXIT=0
…umber that was stale
The reviewer went back over `44d018a` and found two NEW HIGHs, both of them
claims I had written that were not true.
**The F5 pin could not fail.** It read `if category[0] == "L" ... continue` and
then asserted `category != "Lo"`. `"Lo"[0]` is `"L"`, so the continue had
already skipped every `Lo` and the assertion was unreachable for every code
point and every Unicode version — a gate that cannot go red, inside the module
whose whole subject is gates that cannot go red. Replaced with the real
measurement, on both sides: Python pins that every candidate is a code point it
calls unassigned inside one of the twelve viramas' blocks, and the panel's spec
enumerates the same range through `\p{L}` and names the same two. Between them
the divergence cannot change size without a test going red.
**§6 stated a stale measurement as a current one.** It said 98 inputs diverge.
98 was measured against `b59ccbc`, when 69 viramas were live; the narrowing to
12 cut it 24-fold and the number was carried over unchanged — dated, and
presented as measured against the code beside it. It is 4: two code points,
U+0C5C and U+0CDC, both added in Unicode 17, once per joiner. The note now says
how it was wrong, because the way it was wrong is the lesson.
Three more, all mine:
- **The cap could emit a joiner joining nothing.** A joiner is kept because of
what FOLLOWS it, and the cut can put what follows it on the other side — so
`_for_display("ශ්රී", 3)` ended `"ශ්"`, an unescaped joiner as the last
painted character inside the quote, which is clause 1's own shape readmitted
by the capping caller. Both copies now re-escape a trailing joiner after the
cut, where the value's true end is known.
- **`CONJUNCT_PAIRS` claimed to be a census and was a sample.** It asserted its
own length under a docstring saying the cost "cannot grow without somebody
writing the new pair down". Nothing forced that, and two pairs inside the
range were already paying it unlisted: Sinhala ZWNJ — which is the note's own
§5 "kept" vector and is inert in EVERY conformant font, not merely one
lacking a conjunct — and Tamil k+p, which forms no ligature anywhere. Both
added; the length assertion removed, because a count nothing enforces reads
as a bound.
- **`INVISIBLE` was fifteen hand-written characters** under a comment claiming
it was one of two independent statements meeting in the middle, when nothing
compared them. The table has 4174 members. It is parsed from the note now.
Three probes, three reds: the trailing-joiner fix removed; the note's number
set back to 98; the invisible set truncated to one code point per range.
GATE=GREEN EXIT=0 TESTS_RUN=1174 FAILURE_LINES=0 JS_TESTS=262 JS_EXIT=0
A paused run had two doors — the whitelisted `resume_run` the chat panel calls, and
`FlowSession.resume` called in process by another application's worker — and neither said
anything about identity. The remainder of the turn ran as whatever `frappe.session.user`
happened to be, which on the first door is the person ANSWERING.
Measured on all four doors (both are streamed and non-streamed) before it was changed:
- the continuation ran as the answerer everywhere, not only on the Desk door;
- a requester holding no read on `Flow Tool` had a turn that listed 12 of its rows;
- a requester who cannot CREATE a `Flow Tool` had one created for them — reach gained by
asking somebody wider to press a button;
- `_build_prompt_messages` was rebuilt under the answerer too, so the answerer's display
name, time zone and personal notes were eligible to enter the requester's conversation.
The approver is authorising, not performing. They are shown a tool and its arguments; they
are not shown, and never agreed to, "the rest of this conversation runs as you" — while the
loop that follows is steered by the requester's text and continues for up to `max_iterations`
further calls nobody was asked about.
So: authorise as the caller — `assert_run_owner` and `_assert_session_owner` already ran and
stay the caller's business — and PERFORM as the requester, on every door. It fails closed
when the requester is gone or disabled, rather than falling back to the answerer.
The accepted cost, tested rather than hidden: a requester who lacks the permission gets a
refusal after the approval. That refusal used to be EMPTY — a permission check raises an
exception whose own text is blank, so the model was handed `{"error": ""}` and left to invent
a reason. `_failure_sentence` gives it a sentence, without reading the framework's message
log, which names record types this must not put in a prompt.
`flow/tests/test_s25_two_doors_one_identity.py`: 14 tests, every one of them run against all
four doors in the same test, because a check that holds on one door and is never run against
the other is exactly the hole this closes.
GATE=GREEN TESTS_RUN=1188 JS_TESTS=262
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten probes reddened what they should. Three did not, and all three were about this
file rather than the engine:
- `_has_denial -> False` left "a denial creates nothing" green, because the tool still
does not run: `_resolve_confirmation` refuses it call by call. What changes is that the
turn CARRIES ON and the model gets another go at what it was just refused. The test now
counts model calls after the answer — zero on a denial, exactly one on an approval.
- `api.resume_run` dropping its `status != "Paused"` check changes nothing, because
`FlowSession.resume` refuses again underneath. That is defence in depth working; the
probe moved to the deeper guard, which reddens the two in-process doors.
- setting the acting identity as a FIELD, leaving the answerer's permission caches
standing, is an EQUIVALENT mutation, not a surviving one: every cache the check consults
is keyed by the user. Reported as a negative rather than dressed up as a red, with the
answerer's caches warmed first to try to break it. `set_user` is kept for the reason in
`_acting_as` — it is the only thing that also drops the caches that are NOT user-keyed.
Two defects in the test file itself, written down rather than quietly fixed: the streamed
doors were driven by `return_value=<ChatResponse>`, which is not a streaming model double —
the run failed with `'ChatResponse' object is not an iterator` and the streamed sub-tests
passed anyway, because they assert on what the tool recorded, which happens before the model
is consulted again. And no witness was cleared between sub-tests.
GATE=GREEN TESTS_RUN=1188 JS_TESTS=262
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_confirmation_question` built its untemplated body with `json.dumps(call.arguments, indent=2, default=str)`, and `json.dumps` defaults to `ensure_ascii=True`. That dump never passed through `_escaped` at all, so on the commonest card of all -- a gated tool with neither a `confirm_prompt` nor a `confirm_template` -- every non-ASCII character arrived as a backslash-u sequence. Not the joiner run 15 fixed: every letter of every script that is not English. Sinhala, Tamil, Arabic and Chinese were each twenty-odd escapes. WHY THE FLAG WAS THERE, established before it was changed. Nobody chose it: it is a default this call never passed. But run 16 was right that removing it alone is unsafe -- by accident it was the ONLY thing escaping a right-to-left override, a line separator or a hidden joiner on this path. The guard was real. It was indiscriminate: it caught every letter of four writing systems in order to catch three attackers. So the guard is replaced rather than removed, by the stricter rule this module already has. `_readable_arguments` reproduces `json.dumps(indent=2)`'s layout exactly and escapes strings AND keys with `_escaped`, which covers a superset of what `ensure_ascii` was catching and none of the letters. Probe Q3 -- the naive `ensure_ascii=False` and nothing else -- is red on all five guards. The English card is byte-identical, pinned over five argument shapes; what is SENT is byte-identical, asserted through the tool that runs. Two deliberate divergences, both stricter, are named in the docstring. CLAUDE.md rule 4: this run's brief names this function's fallback body, which is the spec the rule requires. ONE line changed; the other three protected functions are byte-unchanged by digest. Both independent pins are re-baselined in the open, each carrying the previous value beside it, following the S19 precedent already in the file. Run 16's two pinning tests are FLIPPED rather than deleted -- its own docstring named them as the tests that must flip. GATE=GREEN TESTS_RUN=1207 JS_TESTS=262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntroduced 1. THE INVERSE ESCALATION, AND IT WAS MINE. T1b argued one direction only: an answerer must not lend permissions to a requester. Pinning the continuation to `run.owner` closes that and opens the other. `flow/triggers/triggers.py` runs a trigger as its configured `run_as`, so a trigger's run is OWNED by a service account -- possibly one holding a posting role, or Administrator -- and pauses whenever `auto_approve` is off. `assert_run_owner` admits a non-owner through `Flow Run` write, which only System Manager holds. Before this run such an answerer ran the remainder as THEMSELVES and was refused; after it they ran it as the service account, for up to `max_iterations` further model calls nobody was shown. That is the answerer gaining reach by pressing a button, which is the sentence the run exists to make false. Worse: an answerer holding `Flow Run` write holds `Flow Session` write too, so the call being approved is one they could have edited. So the rule is the INTERSECTION, not a swap: authorised by the caller, performed as the requester, refused outright when that would hand the caller something they do not have. Role-level, and it says so -- roles are not the whole permission model, so it refuses a superset of what it must and never a subset. Answering your own question never reaches the comparison. 2. THE STREAM LEFT THE IDENTITY INSTALLED ACROSS EVERY YIELD. A generator suspended at a `yield` does not unwind its `with`, and `frappe.session.user` is process-global, so wrapping the whole `yield from` left the requester installed for every frame the CONSUMER ran between events -- the persistence loop, the event serialiser, the WSGI writer. And a client disconnect raises `GeneratorExit` at the consumer's `yield`, so `stream_with_persistence`'s `finally` ran as the requester too. The identity is now entered and left around each advance, and the inner generator is closed as the requester. Two docstring claims the reviewer showed were stronger than the code: `_acting_as` said "four things" and listed three, and `_the_requester` claimed both authorisation checks had already run, which is true of the whitelisted door only. Probes: the reach guard removed -> RED 4 (the four doors of the new test); the stream wrapped whole again -> RED 1. Restored by byte backup, sha256 991f89965484cb33d1b665159dd2277e5a0f91f5c807130c75ecada3172d90d9 both times. GATE=GREEN TESTS_RUN=1212 JS_TESTS=262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A second reviewer, over the previous commit only, found that my role-superset guard refused the Administrator case by ACCIDENT -- and that the accident is removable by the very actor it defends against. `frappe.permissions.get_roles` special-cases Administrator as "every Role row on the site", so `set(get_roles(owner)) - set(get_roles(answerer))` was non-empty only because nobody held every role. A System Manager holds `User` write and may assign themselves every role -- permitted, and no escalation in itself. Once they have, the difference is empty, the guard returns, and the remainder of a trigger's Administrator-owned turn runs as Administrator, whose reach is not made of roles at all: the framework grants it everything unconditionally, a bypass no role can grant. So Administrator is decided BEFORE the arithmetic and never by it. Its test mocks `get_roles` deliberately: the rule must hold independently of what the arithmetic says, because the arithmetic is what an attacker can arrange. Second, the arithmetic failed OPEN on an empty acting user: `frappe.get_roles` discards its argument when `local.session.user` is falsy and answers `["Guest"]` for both sides, so the difference was empty and the guard passed silently -- in the one function that must not. Third, the docstring claimed this "refuses a superset of what it must and never a subset". That is false and is now corrected in place: a User Permission, a document share and an `if_owner` grant all sit under roles, so an answerer with identical ROLES can still lack reach the requester has. It covers roles and nothing else, and says so. Probe R3: both new guards removed -> RED 2. Restored by byte backup, sha256 7245e5bc1b47aebde232127bcac349cac2297fc6dff91de11aa6a5578a577142. GATE=GREEN TESTS_RUN=1215 JS_TESTS=262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rrowed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ance tool confirmation requirements
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Contributor
 [Critical impact] The changes reach approval-gated writes, resumed runs, memory, and record-backed search. Do not merge until the timezone tests and saved usage totals are fixed. Reviews (2) · Last reviewed commit: "Revert "feat(permission): implement perm..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+607
to
+612
| for zone in ("Europe/Kiev", "Asia/Rangoon", "CET"): | ||
| with self.subTest(zone=zone): | ||
| block = self._block(zone) | ||
| self.assertNotIn(zone, block) | ||
| self.assertIn("(UTC)", block) | ||
| self.assertIn("10:42", block) |
Contributor
There was a problem hiding this comment.
Standard Ubuntu timezone data includes Europe/Kiev, Asia/Rangoon, and CET. _resolve_time_zone correctly keeps those names, so these assertions and the system-zone test below fail in CI. Mock ZoneInfo to raise ZoneInfoNotFoundError instead of relying on missing aliases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: flow/flow/doctype/flow_session/test_flow_session.py
Line: 607-612
Comment:
**Tests reject valid timezone aliases**
Standard Ubuntu timezone data includes `Europe/Kiev`, `Asia/Rangoon`, and `CET`. `_resolve_time_zone` correctly keeps those names, so these assertions and the system-zone test below fail in CI. Mock `ZoneInfo` to raise `ZoneInfoNotFoundError` instead of relying on missing aliases.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| # excluded because it is a `Number` that `math.isfinite` will not even accept. | ||
| if not math.isfinite(value): | ||
| continue | ||
| amount: Any = value |
Contributor
There was a problem hiding this comment.
If a custom model reports a cost as Decimal("0.004"), this change keeps it in RunResult.usage. _dump_json saves it as a string, so the next _merge_usage raises when adding another number. The resume fails, potentially after an approved tool has run. Convert retained values to a consistent, JSON-safe number type and test a pause/save/resume round trip.
Prompt To Fix With AI
This is a comment left during a code review.
Path: flow/lib/agent.py
Line: 1681
Comment:
**Decimal costs break saved resumes**
If a custom model reports a cost as `Decimal("0.004")`, this change keeps it in `RunResult.usage`. `_dump_json` saves it as a string, so the next `_merge_usage` raises when adding another number. The resume fails, potentially after an approved tool has run. Convert retained values to a consistent, JSON-safe number type and test a pause/save/resume round trip.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+47
to
+48
| if [ -d "$HOME/code/veyqon-flow/node_modules/vitest" ]; then | ||
| ( cd "$HOME/code/veyqon-flow" && yarn --silent test ) > "$LOG.js" 2>&1 |
Contributor
There was a problem hiding this comment.
The frontend gate always runs ~/code/veyqon-flow, not the checkout containing this script. Other checkout locations fail despite installed dependencies; a second checkout at that path can instead test stale code. Derive the repository root from BASH_SOURCE[0] and run its frontend suite.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/run-tests.sh
Line: 47-48
Comment:
**Gate tests a fixed checkout**
The frontend gate always runs `~/code/veyqon-flow`, not the checkout containing this script. Other checkout locations fail despite installed dependencies; a second checkout at that path can instead test stale code. Derive the repository root from `BASH_SOURCE[0]` and run its frontend suite.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.… and enhance tool confirmation requirements" This reverts commit 7785f05.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.