/kai propose-to-approve knowledge inbox (ADT-235) - #40
Conversation
Add a learning loop where /kai proposes knowledge that the user approves before anything is saved. Proposals live in a separate store that is inert by location — never scanned by the knowledge reader and never reachable by recall — so nothing is auto-applied. Approving re-authorizes the proposal by id (a forged, stale, or already-decided id is refused with nothing written), writes the content into the chosen project or common vault through the same realpath-contained, guarded, no-overwrite, size-capped write, and audits the decision; rejecting is retained for audit and never recalled. The chosen scope is a server-validated enum, never a client path. The Cockpit shows a minimal inbox — approve (project or common) or reject — with the proposal text rendered escaped. Refs: ADT-235
There was a problem hiding this comment.
Pull request overview
Implements a gated “/kai propose → human approve” knowledge learning loop by adding a pending-proposals inbox to the cockpit UI and new hub routes/storage to record proposals inert-by-location until explicitly approved into a fixed scope vault.
Changes:
- Adds a cockpit inbox UI to review pending knowledge proposals and approve/reject them with a fixed scope choice.
- Extends the cockpit control plane and models to support proposal APIs and state projection updates.
- Adds hub-side proposal storage (
~/.aidevteam/kb-proposals), newkb/propose|approve|rejectroutes, and tests validating guard + inertness properties.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| studio/cockpit/src/app/shell/propose-inbox.component.ts | New propose-inbox UI component for approving/rejecting pending proposals. |
| studio/cockpit/src/app/shell/propose-inbox.component.spec.ts | Unit tests for propose-inbox rendering, XSS escaping, scope choice, and API calls. |
| studio/cockpit/src/app/shell/glyph.component.ts | Adds a new propose glyph for the inbox header. |
| studio/cockpit/src/app/shell/base-panel.component.ts | Renders the propose-inbox above the knowledge list when proposals exist. |
| studio/cockpit/src/app/shell/base-panel.component.spec.ts | Tests base panel visibility + state lifting behavior for the inbox. |
| studio/cockpit/src/app/core/models.ts | Introduces KnowledgeProposal and surfaces proposals/counts on KnowledgeView. |
| studio/cockpit/src/app/core/control-plane.service.ts | Adds approveProposal / rejectProposal mutations. |
| studio/cockpit/src/app/core/control-plane.service.spec.ts | Tests new control-plane mutations and inclusion in scoping behavior. |
| hub/lib/state.js | Surfaces pending proposals in the knowledge projection with a proposals count. |
| hub/lib/proposals.js | New proposal store + approve/reject logic (inert-by-location pending store). |
| hub/lib/api.js | Adds kb/propose, kb/approve, kb/reject route handlers. |
| hub/test/proposals-api.test.js | API contract tests for propose/approve/reject and projection behavior. |
| hub/test/mutation-guard.test.js | Ensures propose/approve/reject are refused without guard / non-loopback host. |
| docs/sprints/sprint-06-knowledge-scopes/reviews/rev-knowledge-235.md | Added internal review artifact for ADT-235. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async approve(p: KnowledgeProposal): Promise<void> { | ||
| if (this.phaseFor(p.id) === 'busy') return; | ||
| const scope = this.chosen(p); | ||
| this.setPhase(p.id, 'busy'); | ||
| const res = await this.cp.approveProposal(p.id, scope); | ||
| if (res.ok === true) { | ||
| this.announce(`Proposal approved as ${this.scopeLabel(scope)}.`); | ||
| if (res.state) this.applied.emit(res.state); | ||
| } else if (res.ok === 'conflict') { | ||
| this.setPhase(p.id, 'idle'); | ||
| if (res.state) this.applied.emit(res.state); | ||
| } else { | ||
| this.fail(p.id, res.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved in 30d4e6c — approve/reject now reset the per-card busy phase on success (so a card can't get stuck when the returned state is null or the parent doesn't re-feed), still emitting fresh state when present; the per-proposal scope radiogroup now has roving tabindex + ArrowLeft/Right/Up/Down nav scoped to the focused card (matching the base-panel toggle); and the unused computed import is removed.
| async reject(id: string): Promise<void> { | ||
| if (this.phaseFor(id) === 'busy') return; | ||
| this.setPhase(id, 'busy'); | ||
| const res = await this.cp.rejectProposal(id); | ||
| if (res.ok === true) { | ||
| this.announce('Proposal rejected.'); | ||
| if (res.state) this.applied.emit(res.state); | ||
| } else if (res.ok === 'conflict') { | ||
| this.setPhase(id, 'idle'); | ||
| if (res.state) this.applied.emit(res.state); | ||
| } else { | ||
| this.fail(id, res.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved in 30d4e6c — approve/reject now reset the per-card busy phase on success (so a card can't get stuck when the returned state is null or the parent doesn't re-feed), still emitting fresh state when present; the per-proposal scope radiogroup now has roving tabindex + ArrowLeft/Right/Up/Down nav scoped to the focused card (matching the base-panel toggle); and the unused computed import is removed.
| @@ -0,0 +1,266 @@ | |||
| import { ChangeDetectionStrategy, Component, computed, inject, input, output, signal } from '@angular/core'; | |||
There was a problem hiding this comment.
Resolved in 30d4e6c — approve/reject now reset the per-card busy phase on success (so a card can't get stuck when the returned state is null or the parent doesn't re-feed), still emitting fresh state when present; the per-proposal scope radiogroup now has roving tabindex + ArrowLeft/Right/Up/Down nav scoped to the focused card (matching the base-panel toggle); and the unused computed import is removed.
| <button | ||
| type="button" | ||
| class="seg__opt" | ||
| [attr.data-testid]="'proposal-scope-' + p.id + '-project'" | ||
| role="radio" | ||
| [attr.aria-checked]="chosen(p) === 'project'" | ||
| [class.seg__opt--on]="chosen(p) === 'project'" | ||
| [disabled]="phaseFor(p.id) === 'busy'" | ||
| (click)="choose(p.id, 'project')" | ||
| > | ||
| <dart-glyph name="scope-project" [size]="12" /> This project | ||
| </button> |
There was a problem hiding this comment.
Resolved in 30d4e6c — approve/reject now reset the per-card busy phase on success (so a card can't get stuck when the returned state is null or the parent doesn't re-feed), still emitting fresh state when present; the per-proposal scope radiogroup now has roving tabindex + ArrowLeft/Right/Up/Down nav scoped to the focused card (matching the base-panel toggle); and the unused computed import is removed.
| <button | ||
| type="button" | ||
| class="seg__opt" | ||
| [attr.data-testid]="'proposal-scope-' + p.id + '-common'" | ||
| role="radio" | ||
| [attr.aria-checked]="chosen(p) === 'common'" | ||
| [class.seg__opt--on]="chosen(p) === 'common'" | ||
| [disabled]="phaseFor(p.id) === 'busy'" | ||
| (click)="choose(p.id, 'common')" | ||
| > | ||
| <dart-glyph name="scope-common" [size]="12" /> Common | ||
| </button> |
There was a problem hiding this comment.
Resolved in 30d4e6c — approve/reject now reset the per-card busy phase on success (so a card can't get stuck when the returned state is null or the parent doesn't re-feed), still emitting fresh state when present; the per-proposal scope radiogroup now has roving tabindex + ArrowLeft/Right/Up/Down nav scoped to the focused card (matching the base-panel toggle); and the unused computed import is removed.
approve/reject now clear the per-card busy state on success so a card can't get stuck when the state isn't re-fed or comes back null; the per-proposal scope radiogroup gets roving tabindex + ArrowLeft/Right/Up/Down navigation matching the base panel; drop the unused computed import.
| const docs = []; | ||
| for (const d of own) docs.push({ name: d.name, file: d.file, scope: 'project', stack: d.stack, kind: d.kind, status: d.status, index: 'indexed' }); | ||
| for (const d of visibleCommon) docs.push({ name: d.name, file: d.file, scope: 'common', stack: d.stack, kind: d.kind, status: d.status, index: 'indexed' }); | ||
|
|
||
| const configured = embedderConfigured(project); | ||
| // The /kai inbox: PENDING proposals only. These are inert BY LOCATION (a separate |
There was a problem hiding this comment.
Resolved in e68c015 — buildKnowledge now emits an explicit display annotation when a project note shadows a same-named common note (project doc authoritative:true, common doc shadowed:true/shadowedBy:'project'); both still appear in docs[] (no suppression — the real scope boundary stays scopeMatches). The N-232 test now asserts the marker as its title claims.
| // ---- N-232: precedence is annotation, not suppression ---------------------- | ||
|
|
||
| test('N-232 a conflicting project + common note BOTH surface; the project one is flagged authoritative', async () => { |
There was a problem hiding this comment.
Resolved in e68c015 — buildKnowledge now emits an explicit display annotation when a project note shadows a same-named common note (project doc authoritative:true, common doc shadowed:true/shadowedBy:'project'); both still appear in docs[] (no suppression — the real scope boundary stays scopeMatches). The N-232 test now asserts the marker as its title claims.
… only) When a project note shadows a same-named common note, the projection marks the project note authoritative and the common note shadowed (shadowedBy: project) - a display annotation. Neither note is suppressed; both remain in docs[], and the real scope boundary (scopeMatches) is unchanged.
| function clampText(raw, max) { | ||
| return String(raw == null ? '' : raw).replace(CONTROL_CHARS, '').slice(0, max); | ||
| } |
There was a problem hiding this comment.
Resolved in 0aef662 — clampText now strips every control char via a dedicated global regex (the non-global one is kept only for .test membership checks to avoid lastIndex state); and approve() now flips the proposal to a decided status durably (persistRecord throws on failure, vault untouched) BEFORE the vault write, so a record-write failure leaves it safely pending (nothing written) and a vault-write failure leaves it non-re-approvable — a retry can never produce a duplicate doc. Tests cover both injected-failure paths.
| // Persist the decided record (retained) and append an audit entry to the project's | ||
| // append-only comment trail. The store dir already exists (the pending record lives | ||
| // there). A failed audit append must not undo the decision. | ||
| function persistDecision(projectDir, proposal, summary) { | ||
| const dir = proposalsDir(false); | ||
| if (dir) { | ||
| try { writeRecord(dir, proposal); } catch { /* leave the prior record */ } | ||
| } |
There was a problem hiding this comment.
Resolved in 0aef662 — clampText now strips every control char via a dedicated global regex (the non-global one is kept only for .test membership checks to avoid lastIndex state); and approve() now flips the proposal to a decided status durably (persistRecord throws on failure, vault untouched) BEFORE the vault write, so a record-write failure leaves it safely pending (nothing written) and a vault-write failure leaves it non-re-approvable — a retry can never produce a duplicate doc. Tests cover both injected-failure paths.
…window clampText now strips every control character, not just the first; approve marks the proposal decided before the vault write so a record-write failure can't leave a re-approvable pending proposal that double-writes the note.
| function sanitizeRecord(obj) { | ||
| const out = {}; | ||
| for (const k of Object.keys(obj)) { | ||
| if (FORBIDDEN_KEYS.has(k)) continue; | ||
| out[k] = obj[k]; | ||
| } | ||
| if (!isSafeId(out.id)) return null; | ||
| if (typeof out.status !== 'string') return null; | ||
| out.title = typeof out.title === 'string' ? out.title : ''; | ||
| out.content = typeof out.content === 'string' ? out.content : ''; | ||
| out.why = typeof out.why === 'string' ? out.why : ''; | ||
| out.suggestedStack = Array.isArray(out.suggestedStack) ? out.suggestedStack.filter((t) => typeof t === 'string') : []; | ||
| return out; | ||
| } |
There was a problem hiding this comment.
Resolved in f2315df — sanitizeRecord now SKIPS a record missing required fields (id, content) by returning null (not listed, not approvable), matching the 'malformed record is skipped' contract; optional fields (title/why/source) stay tolerant. reject_ now persists via the throwing path and returns an error on failure (no more false ok:true with the record left pending). Also de-binarized the test file (literal NUL → \x00).
| async function reject_(projectDir, input = {}) { | ||
| const { id, by, note } = input; | ||
| const proposal = loadPending(id); | ||
| if (!proposal) return reject(404, 'proposal not found'); | ||
| proposal.status = 'rejected'; | ||
| proposal.decidedBy = clampText(by || 'user', 64); | ||
| proposal.decidedAt = new Date().toISOString(); | ||
| if (note != null) proposal.note = clampText(note, MAX_WHY_LEN); | ||
| persistDecision(projectDir, proposal, note ? `rejected: ${proposal.note}` : 'rejected'); | ||
| return { ok: true, proposal }; | ||
| } |
There was a problem hiding this comment.
Resolved in f2315df — sanitizeRecord now SKIPS a record missing required fields (id, content) by returning null (not listed, not approvable), matching the 'malformed record is skipped' contract; optional fields (title/why/source) stay tolerant. reject_ now persists via the throwing path and returns an error on failure (no more false ok:true with the record left pending). Also de-binarized the test file (literal NUL → \x00).
…ersist error A proposal record missing required fields (id/content) is now skipped, matching the documented contract, instead of coerced to empty and treated as valid; reject now surfaces a decision-persist failure instead of reporting success while leaving the record pending.
| scopeLabel(scope: KnowledgeScope | undefined): string { | ||
| return SCOPE_LABEL[scope ?? 'project']; | ||
| } |
There was a problem hiding this comment.
Resolved in fd30a31 + e9278a1 — sanitizeRecord now clamps every optional field a tampered record could carry (source→string, suggestedScope→{project,common} with global→common, suggestedKind→valid enum, stack normalized), reusing the propose-time normalizers; and the inbox component independently clamps any out-of-enum/undefined scope to 'project' for both the selected radio and the Approve label, so it can never render 'Approve as undefined' or leave the control unusable.
| /** The scope that will be sent on approve: the operator's choice, else the suggested scope, else project. */ | ||
| chosen(p: KnowledgeProposal): KnowledgeScope { | ||
| return this.choiceById()[p.id] ?? p.suggestedScope ?? 'project'; | ||
| } |
There was a problem hiding this comment.
Resolved in fd30a31 + e9278a1 — sanitizeRecord now clamps every optional field a tampered record could carry (source→string, suggestedScope→{project,common} with global→common, suggestedKind→valid enum, stack normalized), reusing the propose-time normalizers; and the inbox component independently clamps any out-of-enum/undefined scope to 'project' for both the selected radio and the Approve label, so it can never render 'Approve as undefined' or leave the control unusable.
| out.title = typeof out.title === 'string' ? out.title : ''; | ||
| out.why = typeof out.why === 'string' ? out.why : ''; | ||
| out.suggestedStack = Array.isArray(out.suggestedStack) ? out.suggestedStack.filter((t) => typeof t === 'string') : []; |
There was a problem hiding this comment.
Resolved in fd30a31 + e9278a1 — sanitizeRecord now clamps every optional field a tampered record could carry (source→string, suggestedScope→{project,common} with global→common, suggestedKind→valid enum, stack normalized), reusing the propose-time normalizers; and the inbox component independently clamps any out-of-enum/undefined scope to 'project' for both the selected radio and the Approve label, so it can never render 'Approve as undefined' or leave the control unusable.
A corrupted/tampered proposal record now has its source clamped to a string and its suggestedScope/suggestedKind clamped to the valid enums on load, matching the documented contract, so out-of-enum values can't reach the inbox.
…he inbox The inbox now maps any out-of-enum/undefined suggestedScope to the safe default (project) for both the selected radio and the Approve label, so a corrupted record can't render "Approve as undefined" or leave the scope control unusable.
| function propose(input = {}) { | ||
| const { title, content, suggestedScope, suggestedStack, suggestedKind, source, why } = input; | ||
| if (typeof title !== 'string' || title.length === 0 || title.length > MAX_TITLE_LEN) return reject(400, 'invalid title'); | ||
| const cErr = contentError(content); | ||
| if (cErr) return reject(400, cErr); | ||
|
|
||
| const dir = proposalsDir(true); | ||
| if (!dir) return reject(400, 'proposal store is not writable'); | ||
|
|
||
| const proposal = { | ||
| id: crypto.randomUUID(), | ||
| status: 'pending', | ||
| title: clampText(title, MAX_TITLE_LEN), | ||
| content, | ||
| suggestedScope: normScope(suggestedScope), | ||
| suggestedStack: normalizeStack(suggestedStack), | ||
| suggestedKind: normalizeKind(suggestedKind), | ||
| source: clampText(source || '/kai', 64), | ||
| why: clampText(why, MAX_WHY_LEN), | ||
| proposedAt: new Date().toISOString(), | ||
| decidedBy: null, | ||
| decidedAt: null, | ||
| }; | ||
| try { writeRecord(dir, proposal); } catch { return reject(400, 'could not record the proposal'); } | ||
| return { ok: true, proposal }; |
There was a problem hiding this comment.
Resolved in 9f0af3e — propose() now validates the EFFECTIVE (post-clamp) title and rejects it if empty or slugifies to empty (reusing the same slugify addKbNote uses); and approve() validates the title is sluggable BEFORE the decided flip, refusing a corrupted unsluggable record cleanly (422, vault untouched, proposal stays pending) instead of flipping-then-losing the content. slugify is now the single source of truth across propose/approve/addKbNote.
| async function approve(projectDir, input = {}) { | ||
| const { id, scope, by } = input; | ||
| if (!SCOPES.has(scope)) return reject(400, 'invalid scope'); | ||
| const proposal = loadPending(id); | ||
| if (!proposal) return reject(404, 'proposal not found'); | ||
|
|
There was a problem hiding this comment.
Resolved in 9f0af3e — propose() now validates the EFFECTIVE (post-clamp) title and rejects it if empty or slugifies to empty (reusing the same slugify addKbNote uses); and approve() validates the title is sluggable BEFORE the decided flip, refusing a corrupted unsluggable record cleanly (422, vault untouched, proposal stays pending) instead of flipping-then-losing the content. slugify is now the single source of truth across propose/approve/addKbNote.
…nst it propose now rejects a title that is empty or slugifies to empty after clamping; approve validates the title is sluggable before flipping the decided state, so a corrupted unsluggable record is refused cleanly instead of becoming decided-but-unwritten. Reuses the existing slugify from the write module (now exported) so the propose-time and approve-time checks agree byte-for-byte with the slug addKbNote derives the note filename from.
| const projectSlugs = new Set(own.map((d) => String(d.name).toLowerCase())); | ||
| for (const d of docs) { | ||
| if (d.scope === 'project' && projectSlugs.has(String(d.name).toLowerCase())) { | ||
| // only authoritative when it actually shadows a same-named common note | ||
| d.authoritative = visibleCommon.some((c) => String(c.name).toLowerCase() === String(d.name).toLowerCase()); | ||
| } | ||
| if (d.scope === 'common' && projectSlugs.has(String(d.name).toLowerCase())) { | ||
| d.shadowed = true; | ||
| d.shadowedBy = 'project'; | ||
| } | ||
| } |
| // audit also lands in the append-only comment trail | ||
| const comments = state.safeExists; // ensure module loaded | ||
| assert.ok(comments); |
A learning loop: /kai proposes knowledge, you approve before it's saved (design /aura+/apex; arch + HARD secops gated).
What
~/.aidevteam/kb-proposals/) — never scanned by the knowledge reader, never reachable by recall. Nothing auto-applies.Security (HARD gate, verified)
Inert-by-location, no-auto-apply (vaults byte-unchanged without approval), BOLA/IDOR re-auth, write-guard on every route, all ADT-223/234 write conditions on approve, escaped untrusted content. Conditions C-220…C-242 / negatives N-220…N-233 all green.
Tests
304 hub + 448 cockpit; build succeeds. Internal /rev passed (0 blocking).
🤖 Generated with Claude Code