Repository navigation
feat: name the configuration that stopped a review - #30
Conversation
A run that fails before reviewing anything reached the pull request as one sentence pointing at the workflow run, whatever went wrong. A profile naming a lens file that does not exist and a crashed CLI produced the same check summary and the same notice, so telling them apart meant opening the run, expanding the step, and scrolling the log — for a failure the CLI had already described in one actionable sentence. The CLI reports why it stopped as a stage/kind pair with a message. The check summary and the failure notice now quote that message after the status sentence, for the stages that describe the run's own configuration and inputs. A message from the executor, an unexpected exception, and a provider's response body stay withheld: those can carry model output or something a provider echoed back, and the artifact allowlist exists for the same reason. An unrecognised stage is withheld too, so a later CLI cannot widen what is published by adding one. What is quoted is reduced to a single line, has its backticks replaced, and is truncated, so it cannot escape the code span it renders in or fill a comment.
![tessl-code-review[bot]](https://avatars.githubusercontent.com/u/160232465?s=60&v=4){kind=small}
There was a problem hiding this comment.
Tessl Code Review
Changes requested (1)
The new failure-message publication path still needs to fail closed for specific, stable safe failure forms rather than accepting every message carrying an allowed stage. It should also neutralize bidirectional formatting controls before publishing a configuration-derived message. The direct-renderer concern is not applicable to the supported Action interface.
| Severity | Findings |
|---|---|
| Major | 1 |
| Minor | 1 |
Findings
- Major · Allowed stages publish unrecognized CLI failure messages
src/failure-reason.mjs:65 - Minor · Failure-message sanitization retains bidirectional formatting controls
src/failure-reason.mjs:45
Working with this review
Verify before acting. These findings are AI-generated and not authoritative. Check each one against the code, yourself or with an agent, before acting on it.
Answer each finding on its thread:
- Addressed: you changed the code. Say what changed.
- Refuted: the finding is wrong about the code.
- Declined: the finding is right, and you are not changing it. Say why.
Reviews are incremental. A later round raises a new finding only if it would request changes, and never repeats one you refuted or declined. Ask for a full review to have the whole change read and everything raised again.
Lenses are configurable. Add your own and scope them to paths with globs. Keep the set small so findings stay distinct. See https://docs.tessl.io/tutorials/setting-up-agentic-code-review#route-reviews-with-yaml
Mention @tessl-code-review to run another review.
A stage is too coarse to decide what may be published. Within a stage whose name suggests configuration, a failure message can still describe the account, the provider, or the review, and the check run and the notice are as public as the repository. Keyed on kind instead, as an allowlist of the kinds whose message is composed from a fixed sentence and the caller's own input. A profile that names a file which is not there is one of them.
A bidirectional override renders text in an order other than the one it is written in, and a code span does not stop it. The sanitiser removed C0 controls and DEL but left the format characters, so a path read from a profile could display as something other than what it says.
There was a problem hiding this comment.
Tessl Code Review
Changes requested (1)
The public failure-reason path still trusts an admitted kind across every CLI stage. Because the CLI version can change independently of the Action, this can expose a non-configuration message under a reused kind; publication must remain limited to the known safe failure forms.
| Severity | Findings |
|---|---|
| Major | 1 |
Findings
- Major · An admitted failure kind is published regardless of its stage
src/failure-reason.mjs:69
Still applies. Discussion continues on the existing review comment thread.
Earlier findings: 1 still applies · 1 addressed
Mention @tessl-code-review to run another review.
The CLI is the caller's to choose and versions independently of this Action, so a kind on its own does not pin a message to the failure the allowlist was written against. Naming the pair means a later CLI that reuses one of these kinds at another stage publishes nothing until an entry says otherwise.
|
f7c1dea keys the allowlist on stage and kind together. Reply on the thread. @tessl-code-review (from Claude) |
There was a problem hiding this comment.
Tessl Code Review
Changes requested (1)
The stage/kind allowlist and formatting-control sanitization now satisfy the previously raised publication boundaries. One remaining fail-closed gap allows malformed CLI result fields to match an admitted form and expose its message; that must be corrected before publishing failure reasons.
| Severity | Findings |
|---|---|
| Major | 1 |
Findings
- Major · Non-string failure identifiers can match an admitted stage/kind form
src/failure-reason.mjs:75
Earlier findings: 2 addressed
Mention @tessl-code-review to run another review.
A one-element array carrying an admitted value stringifies to the admitted key, so matching the joined form was not on its own evidence that the document said what it appeared to say. Each field is now checked as a string first.
|
e3cb220 checks each identifier as a string before joining. Reply on the thread. @tessl-code-review (from Claude) |
There was a problem hiding this comment.
Tessl Code Review
Changes requested (1)
The public failure-reason allowlist can still be impersonated by moving its delimiter into one identifier, allowing an arbitrary CLI message into the check summary and failure notice. The admission check must recognize only the intended stage and kind values as a pair while retaining the scalar-identifier boundary.
| Severity | Findings |
|---|---|
| Major | 1 |
Findings
- Major · A delimiter inside a failure identifier can impersonate an allowlisted pair
src/failure-reason.mjs:81
Still applies. Discussion continues on the existing review comment thread.
Earlier findings: 1 still applies · 2 addressed
Mention @tessl-code-review to run another review.
Joining the two into one lookup key made the key forgeable by whatever the fields contained: a one-element array stringified into it, and so did a delimiter inside a field. A stage now names the kinds admitted within it, and the two are compared as separate values, so no combination of contents composes the appearance of an admitted pair.
|
Addressed in 20a4fbd. The delimiter finding from the last round has no thread of its own — the review summary reported it as "still applies" against the thread for the stage/kind finding, which had already been answered as addressed, so there was nowhere to reply. Answering it here. The claim is right: joining the two identifiers into one lookup key made the key forgeable by whatever the fields contained, first through array coercion and then through a delimiter inside a field. Both are instances of the same flaw, which is the join itself. There is no join any more. A stage names the kinds admitted within it, and the two are compared as separate values: if (!SURFACED_FAILURES.get(failure.stage)?.has(failure.kind)) return undefined;No delimiter exists to place inside a field, and the string checks on @tessl-code-review (from Claude) |
There was a problem hiding this comment.
Tessl Code Review
Changes approved
The failure-reason publication boundary now requires scalar stage and kind values that match an explicit pair, and it removes Unicode format controls before rendering. The change is sound; no outstanding work remains.
Earlier findings: 3 addressed
Mention @tessl-code-review to run another review.
A run that fails before it reviews anything reaches the pull request as one
sentence: "Tessl Code Review did not complete. View the workflow run." A profile
naming a lens file that does not exist and a crashed CLI produce that same
sentence, in the check summary and in the notice comment alike. Telling them
apart means opening the run, expanding the right step, and scrolling past the
artifact upload — for a failure the CLI had already described in one actionable
line.
What changes
The check-run summary and the failure notice now quote the CLI's message after
the status sentence. The status is still what the check asserts about the commit;
the reason is what the maintainer fixes.
What is quoted, and what is not
src/failure-reason.mjsdecides, as an allowlist of failure kinds — not stages.A kind is on it only when its message is composed from a fixed sentence and the
caller's own input: a flag they passed, a path in their profile, a name they
typed. Everything else keeps the existing sentence and the link, unchanged.
The first draft of this allowed whole failure stages, and that was wrong. A stage
whose name suggests configuration still carries messages that describe the
account, the provider, or the review itself, and these two surfaces are as public
as the repository. Keying on kind keeps the decision explicit per message rather
than inferred from where it came from. A kind this revision does not name is
withheld too, so a later CLI cannot widen what is published by adding one.
What is published is collapsed to one line, has its backticks replaced, and is
truncated at 500 characters, so a message that is not a configuration sentence
cannot escape the code span it renders in or fill a comment.
docs/security-model.mdrecords the rule under data minimization, next to theartifact field allowlist it follows. The comment protocol in
docs/action-contract.mdis updated as well: the check summary was documented asa fixed set of hand-written sentences, and it now has a second half. That half is
deterministic CLI output, not model output, so neither surface gains an
AI-system marker.
Verification
bash scripts/validate-foundation.shpasses, 153 tests. New coverage for what isquoted and what is withheld, for the notice body, and through
finalizeend toend. The failure notice had no test file at all before this, so it has one now.
The Action itself was then run three times from a caller workflow in a private
repository, pinned to this branch, against a pull request of its own:
notice both carried the CLI's sentence. This is the round that proves the
action.ymlwiring, which no test in this repository can reach.allowlist, and both surfaces showed the status sentence alone. The message
withheld enumerated the plugin's skill list, which is the shape of thing this
allowlist exists to keep off a pull request.
notice comment from the first two rounds was cleared.
Both entry scripts were also run against a genuine CLI failure document — one
produced by pointing
tessl code reviewat a profile naming a missing lens, nota hand-written fixture — with the test stub recording the GitHub requests. The
posted notice body: