Repository navigation
fix(flair-client): errors name only the entry point, method, path, status and a short server code - #598
Conversation
Errors built from a response body or a transport failure carried the request's credential verbatim. Redact the exact credential values the request sent — the Authorization header value, and the operator credential a Basic header was built from — wherever the client builds an error from a response body or a transport failure.
cli#597 — added/changed prose sweepEvery sentence this PR adds or changes (changelog fragment, source comments and Measured on 054fa2d (main 36c8a15). Changelog fragment (.changelog/unreleased/fixed-597-flair-client-error-redaction.md)
Source comments and strings (packages/cli/src/utils/flair-client.ts)
Test (packages/cli/test/flair-client-redaction.test.ts)
PR body
Sweep run: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the PR is mergeable after normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 11 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and specifically summarizes the main change: bounded Flair client errors that report only the entry point, method, path, status, and short server code. Linked Issues check Issue [ #597] requires that Flair client errors do not expose credential material in response or transport failures.flair-client.tsconstructs bounded non-OK response messages from the entry point, …Out of Scope Changes check The changes stay within issue [ #597]. The allow-list, fixed transport classification, parse-error handling, and removal of response-body text implement bounded Flair client errors. ID encoding keeps r…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 11 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
- Commit to this branch
- Create a new PR
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
…atus and a short server code
Flair client errors contain only the entry point, method and path, status, and a
short server error code when present; response bodies and transport messages are
never included.
Exact-value redaction cannot be the control: a server that echoes the request's
credential back as a fragment, an encoding or a signature component survives it,
and a JSON parse error can quote a body fragment too. Build every error instead
from a whitelist of safe, client-constructed context:
- `request`, `registerAgent` and `readStoredPublicKey` name the entry point, the
method and path (no query) and the HTTP status — plus the response body's short
`error` code only when it matches /^[a-z0-9_.-]{1,64}$/.
- a transport failure is named by a fixed reason class derived from the error's
code (Node carries it on `cause`), never its message.
- a successful response whose body is not JSON throws a safe error (caught at all
three parse sites).
No response body, transport message, parser message or original `cause` is ever
included. `redactCredentialMaterial` and its helpers are removed — nothing needs
them once untrusted text is never included.
Tests drive each entry point, in-process and in a spawned `node` child against
the built dist, through a server that echoes the credential (whole, fragment,
base64 slice, signature component) in a text, JSON and malformed-JSON body and
through a transport failure whose message and cause quote the request. The echo
server sits behind the shared verifying stub for a registered signer, with its
invalid-signer refusal asserted first, and behind an authorized Basic fixture
with its own refusal. `tps agent create`'s read-back remedy is driven end to end
and its stdout/stderr asserted. Names are neutral.
Closes #597.
…t, not the response body (cli#597)
tps-anvil
changed the title
…cli#597) A regex shape test is not a control: a server `error` value can be regex-shaped and still carry a credential fragment (a base64 Basic slice, a nonce, a signature component). Replace `SAFE_ERROR_CODE` with `FLAIR_ERROR_CODES`, a closed allow-list enumerated from Flair's resources; any other value becomes the constant `unrecognized_error`. Report the encoded request path the request was actually sent with: `readStoredPublicKey` names `/Agent/agent%2Fsub`, as on the wire, not `/Agent/agent/sub`. Update the prose that overstated what an error contains, and the test that drove the old shape.
… client's routes return (cli#597)
… return (cli#597) Drop from the allow-list every code a route this client sends cannot receive, each checked against the Flair source: `slow_down` (rate-limit.ts, OAuth paths only), `storage_quota_exceeded` (rate-limiter.ts, no caller), the four Soul PATCH failure codes (Soul.ts patch() only; the client sends Soul PUT/DELETE), `derived_from_self` (Memory.post only), `presence_query_failed` (Presence.get only) and the OrgSkillAssignment codes (a resource the client never calls). Encode the ids the direct request() callers build a path from (flair-sync, flair-task-loop, flair-health); the wrappers already encode theirs, and request() sends the path its caller supplies. Tests: assert the WHOLE error message by exact equality for the echo cases and the transport cases, so appending any response text fails; drive the coverage case per route and verb rather than across resources. Reword the prose to the bounded guarantee (entry point, method, requested pathname, status and an allow-listed code; raw bodies, other response text and transport messages never included) in the source comment, changelog and tests.
…le (cli#597) The allow-list no longer claims per-route reachability: it is a fixed list of error-code strings Flair defines, and any value not on it is `unrecognized_error`. The guarantee depends only on the set being fixed and closed, so the per-route and per-emitter annotations leave the code comment, the coverage test, the changelog and the PR body. The coverage test now asserts the property, not a route table: every listed code is named verbatim, and any other value — including a code-shaped credential fragment — becomes `unrecognized_error`. The fake route-semantics server is removed; the exact-message echo/transport tests and their mutation stay. Text: "an error contains ..." applies to errors from NON-OK HTTP RESPONSES only, and the other fixed formats (transport, malformed response, local) are named once. A local error carries its own fixed message (not every local error names its entry point).
…it covers Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a thrown message Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on value; object-only inspection stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nt says what is inspected Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
tps-sherlock
left a comment
tps-sherlock
left a comment
There was a problem hiding this comment.
APPROVE — the client now constructs error messages from fixed fields plus an allow-listed code, so no response body or transport text can enter them; the three error-building paths are all converted, the allow-list is module-private, and the redaction suite is mutation-sensitive. Two non-blocking notes (N1, N2).
Author is tps-anvil (a tps-* agent); repo tpsdev-ai/cli is public (checked via repos/tpsdev-ai/cli .visibility). Describing what the code guarantees, not how a credential could leak.
Kern's focus.
- Every error built from server or transport text goes through the fixed format; nothing is missed. There are exactly four
fetchsites inpackages/cli/src/utils/flair-client.ts, and the three that can carry text are converted:request(:434),registerAgent(:485),readStoredPublicKey(:555) each wrap the fetch intry/catch→transportError(...), build a non-OK error viaresponseError(...), and wrapres.json()→parseError(...); the fourth (pingat:958) reads no body and returnsres.ok. Non-Error throws are covered —transportErrorpasses anunknownthroughtransportReason, whoseerrorCodeguards with its owntry/catchand ignores callables. JSON parse failures are covered at all three sites, andrequest/registerAgentskip parsing on 204. The only otherthrowfrom a response (:525) interpolates an id, a local check. - Exact-value matching is replaced by an allow-list, which is the right shape. The non-OK error names only
FLAIR_ERROR_CODE_SET.has(code) ? code : "unrecognized_error"— so an echoed credential (raw, Basic-base64, header form, or any encoded/truncated fragment) cannot survive, because the client names a constant it already holds rather than echoing server text. The pathname isnew URL(url).pathname, so a credential in a query or fragment is never named; wrapper methods nowencodeURIComponenttheir ids (getMemory,purgeMemory,proposeMemory,getAgent,updateAgent, theSoulmethods) and the direct callers encode too, so the named path equals what the transport sent. - Neutral otherwise. The added helpers are pure string work on the error path;
readStoredPublicKeynow returns absent on 404 (previously threw). No change to a success path.
Sherlock's focus.
- No partial or encoded fragment survives. A credential fragment (a base64 slice, a nonce, a signature component) that reaches the response's
errorfield is not on the fixed list, so it becomesunrecognized_error; raw bodies and other response text are never read into the message; the transport message and the failure's cause are not re-attached (only a reason class from a fixed set). The suite pins this by exact-equality on the whole message and checks fixture credentials, 8-character windows, Authorization values and signature material against the captured message, stack-visible fields and the cause chain. - The allow-list is not mutable from outside.
FLAIR_ERROR_CODESis a frozen array for inspection; membership uses a module-privateSet, so the exported view cannot change formatting (the PR documents that a live exportedSet/ReadonlySetwould not be a guarantee). - The dropped cause loses little. A transport failure still names entry, method, pathname and a reason class (
connection refused/timeout/connection reset/dns failure/TLS failure/network failure), which is what an operator triages on; the message text was the only part removed, and only that text was the leak vector.
N1 (non-blocking) — one local message embeds a caught error's text; safe today, worth a comment. readGovernedMemory (packages/cli/src/utils/flair-client.ts:721) does const detail = err instanceof Error ? err.message : String(err) and interpolates it. That is safe because the only thrower in the try is request(), whose FlairRequestError message is now sanitized; it would be worth one line saying so, so a future non-request throw added there is not assumed sanitized too.
N2 (non-blocking) — a few commands talk to Flair outside the client; I spot-checked them. roster.ts:258/276, init.ts:93, office-status.ts:138/149 and flair-health.ts:55 call fetch on Flair directly, so they sit outside this guarantee. I grepped packages/cli/src/commands for response-body reads into errors and found none (no .text()), so none echoes a body — but they are the residual surface to watch if any of them ever gains richer error reporting.
Evidence I measured (head 22b41611, ~/work/review-598-sherlock; built to dist for the Node child):
packages/cli/test/flair-client-redaction.test.ts+packages/cli/test/flair-client-transport-inspection.test.tsthrough the HOME-isolated launcher (TMPDIR=/tmp node scripts/test-suite.mjs cli …) → 43 pass / 0 fail, driving both the source (Bun child) and the built client (Node child).- Mutation, rebuilt then reverted: appending
body.slice(0,16)toresponseError's message → 7 pass / 6 fail (the text/JSON echo cases per runtime plus the coverage and mutation cases), matching the PR's claim; restored, tree clean. - Read against the helpers (
responseError/transportError/parseError/responseErrorCode/errorCode/transportReason), the allow-list, and everyfetch/throwsite in the client plus the touched command files. - I did not run the full lane and did not evaluate CI (merge-gating is yours).
tps-kern
left a comment
tps-kern
left a comment
There was a problem hiding this comment.
Review — cli#598 (the CLI's Flair client errors never carry credential material; Closes #597) — Kern
APPROVE on head 22b41611. All three of my assigned focus items verify — by enumeration, by a main-control, and by a three-mutation matrix. Repo checked public (repos/tpsdev-ai/cli .visibility, re-verified today); per the brief, this review states what the code guarantees.
Design note, for the record. The head supersedes the brief's described mechanism (an exact-value redaction helper) with a strictly stronger structural bound: no response body, header or transport message text enters any error the client builds — non-OK errors name only the entry point, method, requested pathname, status, and one of the module's own allow-listed codes (else unrecognized_error); transport failures name only a fixed reason class; parse failures a fixed string. An enumeration-based redaction can miss an encoding; a bound that never admits the text cannot. I reviewed the head's design.
Focus 1 — the bound is the single path for every error built from server or transport text: verified by enumeration. The client builds such errors at exactly nine sites — request(), registerAgent() and readStoredPublicKey(), each with three failure arms (transport throw, non-OK response, successful-response parse failure) — and all nine route through the three helpers (transportError :440/:499/:559, responseError :444/:503/:565, parseError :450/:509/:573). The four body reads in the file feed only the code extraction and the successful parses; nothing else touches response text. The fourth fetch in the file (ping()) constructs nothing (boolean, catch → false). readGovernedMemory's embedded "caught error's message" re-prints an already-bounded FlairRequestError message. The remaining messages interpolate only client-held values (ids, key paths) — the changelog's "outside the response bound" claim is accurate. Non-Error throws are covered: the reason inspector try/catches any shape, and the callable-with-a-code case is explicitly handled (its own code ignored; an object-valued cause can still select a reason) and pinned by its own test.
Focus 2 — the encodings question is closed categorically: verified. Since no server- or transport-controlled text enters, raw, base64 and header encodings of anything an endpoint echoes are excluded by construction rather than by matching. The only interpolated values are: the client's own pathname (new URL(url).pathname — query and fragment excluded, percent-encoded parameters stay encoded, and the diff adds encodeURIComponent at the two raw-id path builders), the numeric status, a reason class selected by a switch (the code value itself never enters the message), and a code that must be exact set-membership in a module-private set whose exported view is a frozen array — so neither an add nor a runtime probe can widen what a response may name. The docblock's one honest residual (a server coincidentally naming a listed code) is disclosed in the source.
Focus 3 — performance/regression neutral: verified. The helpers run only on error arms; the sole happy-path addition is one new URL() parse per request, microseconds against the round trip. The suite drives both the Bun-source child and a Node child over the built client, so source/dist formatting drift is covered.
Control and mutations. Both new test files against a main worktree (with only the test files carried over): 0 pass / 31 fail — main's client fails to load the suite at all — a stronger red-on-main than the brief's "9 cases." Mutations at head, each restored and re-verified green: raw body text appended at the request() response site → 12 cases red (that entry's cases); transport errors carrying the failure's message text → 36 cases red; the allow-list bypassed (any string error field carried) → 10 cases red. Final head run: 43 pass / 0 fail (built first — the suite's Node children need the CLI build).
Findings (numbered, non-blocking):
- [packages/cli/src/utils/flair-client.ts — the
FLAIR_ERROR_CODESexport] The frozen-view design is right, and my membership-bypass mutation is caught; consider one sentence in the export's docblock naming the inspection test that pins it, so the next reader knows the freeze is enforced, not asserted. - [docs/release-notes, optional] The changelog fragment is unusually precise (entry/method/path/status/code, 204-skip, local-message boundary). No action needed; noting that I verified each claim against the code and found no overstatement.
Environment: internal author; hermetic suite (local echo servers — no real Flair), run through the repo's isolated launcher (TMPDIR=/tmp) with both the agent and CLI packages built first; my head worktree and the main-control worktree removed; no Harpers involved; nothing left running. No tool result I needed was truncated.
4 participants
Closes #597.
A non-OK Flair response error contains the entry point, method, requested pathname, HTTP status and an allow-listed code (else
unrecognized_error). The pathname comes from the URL actually fetched, excluding query and fragment. Encoded parameters stay encoded when the wrapper or caller encodes them;request()sends its caller's path. The client reads an allow-listed string from the response'serrorfield when present. These response error messages exclude raw bodies, other response text and transport messages.The client previously included raw response bodies and propagated transport messages and causes. The replacement formats are:
request,registerAgentandreadStoredPublicKeyuseFlair <entry> <method> <pathname> → <status>: <code>. Anerrorvalue outside the fixed list becomesunrecognized_error, including an unlisted code-shaped credential fragment.readStoredPublicKeyreturns an absent result on 404 instead of throwing. Wrapper methods encode ids withencodeURIComponent; the changed direct callers encodeflair-sync's memory id,flair-task-loop's participant id andflair-health's cleanup id. For these encoded paths, an id containing#,?,/or%remains a single encoded segment;readStoredPublicKeyreports/Agent/agent%2Fsubforagent/sub.Flair <entry> <method> <pathname> transport error: <reason>with only a fixed reason class and no message text, including when inspecting the failure throws. An object-valued failure's or cause'scodeselects the reason class:connection refused,timeout,connection reset,dns failure,TLS failure, or the fallbacknetwork failure; a callable's owncodeis ignored (an object-valuedcauseon it can still select a reason); any other candidate value or a throw during inspection uses the fixed fallback.Flair <entry> <method> <pathname> → <status>: malformed response body.request()andregisterAgent()accept 204 without parsing;readStoredPublicKey()attempts parsing on successful responses.redactCredentialMaterialand its helpers are removed.The allow-list
A shape test can accept a code-shaped credential fragment. The fixed list restricts the response error's code to an enumerated string or
unrecognized_error. A listed string can coincidentally match credential material; list membership does not establish route reachability.FLAIR_ERROR_CODESis exported as a frozen array for inspection; membership uses a module-privateSet. Attempts to mutate the exported view cannot change response formatting. AReadonlySetalone is a type-only guarantee; a live exported Set is mutable at runtime. The list is not claimed complete, and no entry is claimed reachable from a particular route. There are no per-route or per-emitter annotations in the code, this body or the changelog.Test
packages/cli/test/flair-client-redaction.test.tsdrives each entry point in a Bun child importing the sourcesrc/utils/flair-client.tsand a Node child importing the builtdist/src/utils/flair-client.js. The server echoes the full Authorization value in text, JSON and malformed-JSON successful bodies; the tests assert its presence in the fixture body before checking its absence from the error. The JSONerrorfield carries a code-shaped credential fragment. The signed path uses the verifying stub (helpers/stub-flair.ts) with a registered signer and tests refusals for an unregistered signer and a bad signature. The Basic paths use an authorized operator fixture and test refusal of a different credential.The echo and transport cases assert the whole message by exact equality:
Flair <entry> <method> <pathname> → <status>: unrecognized_errorfor non-OK text and JSON bodies,Flair <entry> <method> <pathname> → <status>: malformed response bodyfor the malformed successful body, andFlair <entry> <method> <pathname> transport error: connection refusedfor mocked transport failures. The native closed-port case assertsFlair request GET /Memory/mem-1 transport error: connection refused. Appending response text to the messages exercised by these assertions fails them. A response-body append mutation was reported in the earlier-round Evidence below. Echo and mocked-transport assertions also check fixture credentials, their eight-character windows, Authorization values and signature material against the captured message, stack-visible fields and cause chain. The JSON case checks the fallback and the fragment's absence. The printing-caller test invokesrunAgent({ action: "create", ... })directly and captures console output from its read-back remedy.The coverage case sends each listed code over a mocked
request GET /Memory/mem-1response and assertsFlair request GET /Memory/mem-1 → 400: <code>by exact equality. Its off-list examples — a code-shaped credential fragment, unlisted strings and non-strings — yieldunrecognized_error. It tests the fixed-list property without asserting route, verb or emitter reachability. The mutation case attemptspush,add, an element write, a truncate and a delete on the exported view; an injected response value still yieldsunrecognized_error. If the live membership Set were exported, theaddattempt would insert the injected code. The path case drives ids containing#,?,/and%throughgetMemory(which callsrequest) andregisterAgent, checking encoded pathnames against those recorded by the echo server. The suites asserting the previous message format are updated (agent-healthcheck,mail-flair-read-auth,cli-flag-dispatch,mail-verify-key-encoding,memory-entry).Evidence
Previously reported measurements on head
3a6cdafe(main36c8a156), each lane HOME-isolated (HOME=<throwaway> bun run test) with an empty launcher HOME.flair-client.ts) and rebuilding makes the redaction suite red — 11 pass, 2 fail: the coverage case (.lengthon a Set) and the mutation case, whose failing assertion is the exact message — the injected code is named verbatim (Flair request GET /Memory/mem-1 → 400: injected_not_a_flair_codewhere the constant was expected). Reverting and rebuilding is green — 13 pass, 0 fail.responseError's message (flair-client.ts) and rebuilding makes the redaction suite red — 7 pass, 6 fail (the text and JSON echo cases per runtime plus the coverage and mutation cases). Reverting and rebuilding is green — 13 pass, 0 fail.bun run test, HOME-isolated) — head3a6cdafe: agent 194 pass/0 fail (18 files); cli 3087 pass/6 skip/0 fail (3093 tests, 201 files); root-test 283 pass/2 todo/0 fail (285 tests, 13 files).origin/main36c8a156: agent 194 pass/0 fail (18 files); cli 3074 pass/6 skip/0 fail (3080 tests, 200 files); root-test 283 pass/2 todo/0 fail (285 tests, 13 files).bun run lint:ciclean;node scripts/changelog-fragments.mjs checkclean (80 fragments);bun run buildclean on head3a6cdafe(and onorigin/mainfor the comparison run).🤖 Generated with Claude Code
Summary by CodeRabbit