fix(cli): memory governance fails closed; search signs as the operator; approve/reject refuse (#509) - #514
Conversation
…r, approve/reject refuse (#509) - archive/unarchive read the record first and refuse, without a write, when that read fails or is incomplete; the update is a server-side PATCH carrying only the governance fields. - search signs as TPS_AGENT_ID and passes the target agent as the search's agentId parameter; it refuses when that identity is unset. - approve/reject exit non-zero: Flair exposes no operation that promotes or rejects a memory by id.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CLI now signs memory search as the operator and passes the target agent as a search parameter. Archive and unarchive require a complete record before sending governance-only PATCH requests. Approve and reject refuse because promotion by memory ID is unsupported. ChangesMemory governance and identity
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🔵 Low · up to Archive and unarchive are mergeable with owner awareness of a rare race: a concurrent deletion can leave an incomplete memory record. An update-only server operation would eliminate it. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change tightens search identity and reduces write scope. A privileged archive operation can nevertheless race a deletion and recreate an incomplete record on the server version examined. The risk is narrow, and the deployed server version remains unconfirmed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
cli#509 sweep — added/changed prose claim audit (head 89f29ab)Method:
Result: no over-broad sentence found; no change required beyond the wording Not fixed / noted: the full cli suite has unrelated, pre-existing failures |
|
Adjudication notes for round 2, all public facts from Flair main:
|
…ard wording - memory-entry: assert the CLI propagates a SemanticSearch refusal (a non-admin search whose target differs from the signing principal) and a Memory.patch refusal, each as a non-zero exit with no success output. - Drop the "refused for every caller" characterization of the promotion guard from the approve/reject error message and the test comment. - Describe the search agentId as the reader whose scope is resolved, not as an owner filter.
cli#509 sweep — round-2 added/changed prose (head 50f5489)Method:
Removed over-broad sentences (findings 2 and 3), no replacement claim added:
Guarantee-word grep over this round's added lines: no hits. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
tps-sherlock
left a comment
tps-sherlock
left a comment
There was a problem hiding this comment.
APPROVE — cli#514, head 50f5489.
Repo visibility checked: tpsdev-ai/cli is public (gh api repos/tpsdev-ai/cli → .visibility: "public"). This review adds no unpatched-bypass detail — it confirms behaviour the diff already states.
Reviewed the diff, built the worktree (bun install --frozen-lockfile && bun run build, exit 0), and ran the changed suites through the repo's isolated launcher (node scripts/test-suite.mjs cli … with TMPDIR=/tmp). No production process was touched; I started no servers.
What I verified, with the code and the test that fails without it:
1. Search signs as the operator, and the target rides as the search's agentId parameter. src/commands/memory.ts:143-147:
const signer = requireLocalAgentId("memory operator id", process.env.TPS_AGENT_ID);
const searchClient = createFlairClient(signer, flairUrl, args.keyPath ?? defaultFlairKeyPath(signer));
const results = await searchClient.search(args.query, args.limit ?? 10, { agentId: args.agentId });and src/utils/flair-client.ts:353-357 carries the target through:
async search(query: string, limit = 5, opts: { agentId?: string } = {}): Promise<SearchResult[]> {
...
{ agentId: opts.agentId ?? this.agentId, q: query, limit },Mutations: reverting the signer to args.agentId (the pre-#509 behaviour) fails "signs as TPS_AGENT_ID and carries the target as the agentId parameter" and "refuses when TPS_AGENT_ID is unset"; dropping the { agentId: args.agentId } argument fails the first. So the CLI no longer signs with the target's key — the only key path used is defaultFlairKeyPath(signer), and the dispatch (packages/cli/bin/tps.ts:1395-1410) passes no keyPath, so an operator cannot point the search at another agent's key.
I checked the counterpart in Flair, since the CLI comment defers to it: resources/SemanticSearch.ts:105-118 rejects a non-admin whose body agentId differs from the authenticated principal ("forbidden: agentId must match authenticated agent") and resolves the read scope from the authenticated agent, not the body. So "Flair's read scoping decides visibility" is enforced server-side, and the CLI test that models that 403 ("search propagates a SemanticSearch refusal as a non-zero exit with no results output") is the right control.
2. A failed or incomplete read never reaches a write. src/utils/flair-client.ts:428-456:
private async readGovernedMemory(id: string): Promise<Memory> {
try {
record = await this.request<Memory>("GET", `/Memory/${encodeURIComponent(id)}`);
} catch (err) {
...
throw new Error(`refusing to update memory ${id}: the read failed (${detail})`);
}
if (!record || typeof record.id !== "string" || typeof record.agentId !== "string" || typeof record.content !== "string") {
throw new Error(`refusing to update memory ${id}: Flair returned an incomplete record`);
}private async patchGovernedMemory(id: string, patch: Record<string, unknown>): Promise<void> {
await this.readGovernedMemory(id);
await this.request("PATCH", `/Memory/${encodeURIComponent(id)}`, patch);
}Mutations: deleting the completeness gate fails "archive: an empty read sends no write and refuses"; swallowing the read failure and fabricating a record fails the 503 and 404 cases. So a failed/incomplete read cannot be followed by a write.
3. The PATCH body is only the governance fields — no id. archiveMemory/unarchiveMemory (:466, :474) call patchGovernedMemory(id, { archived, archivedBy, archivedAt }). Mutation: adding id to the body fails all four PATCH-shape assertions (governance-509 and memory-cli, archive + unarchive). I also confirmed against Flair that archived/archivedAt/archivedBy are not in AUTHORITY_FIELDS (resources/authority-field-guard.ts:5, which lists only promotionStatus/promotedAt/promotedBy), so the body is accepted, and Memory.patch (resources/Memory.ts:949) merges server-side — the docstring's claim holds.
4. approve/reject send no write and name what is missing. src/utils/flair-client.ts:145-151, 458-464:
function promotionUnsupported(action: "approve" | "reject", id: string): Error {
return new Error(
`tps memory ${action} is unavailable: Flair has no operation that sets a memory's promotion status by id. ` +
`Promotion uses Flair's candidate workflow (POST /PromoteMemoryCandidate, on a MemoryCandidate id). ` +
`${id} was not changed.`,
);
}
...
async approveMemory(id: string): Promise<void> { throw promotionUnsupported("approve", id); }The diff also removes the two console.log success lines from memory.ts (they would have printed after a now-throwing call). Mutation: making approveMemory a no-op fails "approve refuses by name and sends no write" (both files). This is why a direct promotionStatus PATCH is not an alternative — Flair's guard returns 403 for that (authority-field-guard.ts:15-19), matching the issue.
5. No success output after a refused write. archiveMemory/unarchiveMemory are awaited before their console.log (memory.ts:86-94), and request() throws on any non-2xx (flair-client.ts:207-220), so the success line is unreachable on failure. The entry tests assert exactly this: "archive propagates a Memory.patch refusal as a non-zero exit with no success output" and the search 403 case both expect status === 1 and stdout.trim() === "".
Tests run (all green, isolated HOME via the launcher): memory-governance-509.test.ts, memory-cli.test.ts, memory-entry.test.ts, no-identity-defaults-499.test.ts — 36 pass / 0 fail. I did not run the whole cli suite, and I make no claim about any lane I did not run.
Non-blocking notes (no change requested):
- With the fix,
tps memory search <otherAgent> <q>will 403 for an operator Flair does not treat as admin, because the bodyagentIdno longer matches the signer. That is the intended read-scoping outcome (the FOCUS says so), but it does change the command's usefulness for a non-admin operator — worth a line in the command's--helpat some point. - The top-of-command signer fallback is unchanged:
memory.ts:47isrequireLocalAgentId("memory operator id", process.env.TPS_AGENT_ID ?? args.agentId). The dispatch never setsagentIdforarchive/unarchive/approve/reject(targetsAgentis["review","list","search"]), so this PR's write paths cannot be signed as a target — butreview/liststill fall back to signing asargs.agentId, which is the pre-existing #499 behaviour the issue left in place. Flagging it only so the next reviewer knows it was considered, not overlooked. - The CLI tests assert the PATCH shape; that the merge drops nothing is Flair's property (read from
Memory.patch), not something a CLI mock can prove. Stated so the claim isn't read as CLI-tested.
Verdict: APPROVE.
tps-kern
left a comment
tps-kern
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed head 50f5489a (re-verified unchanged at post time). Repo visibility checked and confirmed public via repos/tpsdev-ai/cli (.visibility: "public", private: false); this review stays at the disclosure level of the diff itself.
Method: full diff read (7 files); clean worktree build (bun install --frozen-lockfile && bun run build — green); the three memory suites run through the repo's own isolated lane launcher (node scripts/test-suite.mjs lane packages/cli/test/memory-governance-509.test.ts packages/cli/test/memory-cli.test.ts packages/cli/test/memory-entry.test.ts) → 32 pass / 0 fail. Results are from my own worktree run, not CI — I make no claim about CI lanes. Flair-side contract claims were verified by read-only grep of the production install (/opt/homebrew/lib/node_modules/@tpsdev-ai/flair): patchRecord merges ({ ...existing, ...patch }), guardAuthorityFields guards Memory: ["promotionStatus", "promotedAt", "promotedBy"], and PromoteMemoryCandidate acts on a MemoryCandidate id — every premise the PR relies on holds there.
Focus verified
1. A failed or incomplete read never leads to a write. readGovernedMemory (packages/cli/src/utils/flair-client.ts:428) gates every governance update: a GET that throws becomes refusing to update memory <id>: the read failed (…) and a 200 body without string id/agentId/content becomes … Flair returned an incomplete record — both before any write exists to send. patchGovernedMemory (flair-client.ts:453) is the only write path left: read gate, then PATCH. The old GET-merge-PUT patchRecord is gone, which also closes the read-then-write race and the projected-read rewrite hazard. Pinned by: 503/404 read → rejects + calls === [] and empty-read → incomplete record + no calls (packages/cli/test/memory-governance-509.test.ts:52-71), and the entry test where a modeled Memory.patch 400 refusal exits 1 with empty stdout (packages/cli/test/memory-entry.test.ts).
2. The PATCH body carries only the governance fields (no id). archiveMemory sends {archived: true, archivedBy, archivedAt} (flair-client.ts:466-472); unarchiveMemory sends {archived: false, archivedBy: null, archivedAt: null} (flair-client.ts:474-480). Tests assert the body's keys are exactly ["archived", "archivedAt", "archivedBy"] for both paths (memory-governance-509.test.ts:73-100, memory-cli.test.ts archive/unarchive) — no id, nothing else — and Flair's patchRecord merge (verified above) means unprojected fields can no longer be dropped by a client-side merge.
3. Approve/reject never send a write and name what is missing. promotionUnsupported (flair-client.ts:145-151) throws before any request: it names the missing operation (no by-id memory promotion), names the real path (POST /PromoteMemoryCandidate, on a MemoryCandidate id), and states <id> was not changed. The success log lines are removed from memory.ts:72-83. Pinned by memory-governance-509.test.ts:131 and memory-cli.test.ts: both actions reject with the named reason and the captured-call list is empty.
4. Search signs as the operator (Sherlock's lane, cross-checked). The old code built the client as the target (createFlairClient(args.agentId, …)) — the operator signed with the target's key. Now the signer is strictly TPS_AGENT_ID (memory.ts:145, refused when unset with no agentId-arg fallback), the key path is the operator's (memory.ts:147), and the target rides as the search's agentId parameter (flair-client.ts:353-360), leaving read scoping to Flair. Pinned by: auth TPS-Ed25519 operator: with body.agentId == "target-a" (memory-governance-509.test.ts:102), refusal + zero calls with TPS_AGENT_ID unset even though the agentId argument is present (:117), and the entry test where Flair's scoping refusal (403) exits 1 with empty stdout. The #499 changelog line was correctly narrowed (search removed from the agent-id-fallback list) — the changelog matches the code.
5. No command reports success after a refused or failed write. Success lines print only after the awaited write resolves; every refusal path prints to stderr and exits non-zero — asserted at the real bin level by the entry suite (non-zero status + stdout empty for search and archive refusals, plus per-subcommand "refuses without an operator identity").
Every behavior claim in the diff's comments and changelog names a test that would fail without it (checked pairwise above).
Observations (non-blocking, numbered)
packages/cli/src/commands/memory.ts:47vs:145— the shared client resolves the operator asTPS_AGENT_ID ?? args.agentIdwhile the search case re-requires strictlyTPS_AGENT_ID. The strictness difference is intentional and tested (search refuses even when the agentId argument is present), but the double resolution means the shared client is constructed with the target as its identity before the search case throws. No request is sent on that path; noting the coupling only.packages/cli/src/utils/flair-client.ts—purgeMemoryremains a direct DELETE with no read gate. That is outside #509's scope (server-side admin gate, and the entry suite pins the identity refusal), noted for completeness.
Could not see
Nothing in the diff was left unread. Two process notes, disclosed: (a) my first background build emitted one transient tsc line (mail.ts:692, TS18046 — a file this PR does not touch); it did not reproduce on two clean re-runs (exit 0) and the gate build (the script's final un-||true step) passed both times — I treat it as a worktree artifact, not a PR defect. (b) My first attempt to run the suites used a wrong launcher argument (a bare suite name), which the launcher silently widened to a broad default lane; I killed exactly that run (pids 89577/89580) and re-ran the three files through the correct lane invocation. All test evidence above is from the corrected run.
4 participants
Closes #509
Three follow-ups to #501's
tps memorygovernance commands.with no write sent, when the read fails (any non-2xx) or returns an incomplete
record (no
id,agentId, orcontent). The update is a server-sidePATCH /Memory/<id>carrying onlyarchived,archivedBy, andarchivedAt.tps memory searchsigns withTPS_AGENT_IDand refuses when it is unset; the target agent goes as thesearch's
agentIdparameter and Flair scopes the read. The.changeloglinethat said
searchcould sign with its agent-id argument is corrected.rejects a memory by id; its promotion operation
(
POST /PromoteMemoryCandidate) acts on aMemoryCandidateid. Both commandsexit non-zero with that message; neither sends a write.
Evidence, measured on 89f29ab (main 70bccde):
packages/cli/test/memory-governance-509.test.ts: red on main (0 pass, 9fail); green on 89f29ab (9 pass, 0 fail).
memory-governance-509,memory-cli,memory-entry,memory-learn): 36 pass, 0 fail on 89f29ab.cd packages/cli && bun run lint:ci: exit 0.node scripts/changelog-fragments.mjs check: 43 fragments, OK.bunx tsc --noEmitunderpackages/cli): exit 0.unrelated failures (identical names: plugin node-load / gateway-boundary,
WsNoiseTransport, Stall Monitor, roster invite, launcher isolation) plus the
nine above — none touches a memory file.
Each new test was mutation-checked: with the fix broken the test fails, and the
file is restored to green. The Flair side of the seam is exercised by fakes
whose request and response shapes are read from Flair main's
resources/Memory.ts(patch()merges) andresources/SemanticSearch.ts(thebody
agentIdfield), not a live Harper.Summary by CodeRabbit