Repository navigation
✨ retro: show owners review-derived lessons awaiting approval - #14738
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Co-authored-by: Danathar <6772335+Danathar@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
There was a problem hiding this comment.
Read the handler, its helpers and the producer side at v6; this looks correct to me.
- The owner gate is the same verified-role check the package already uses:
knowledgeListStates(api_knowledge.go:217-231) callsrequireOwnerRole(api_agent_control.go:520-527), which demands bothX-Hive-Roleowner and the server-only verified marker — identical to whatknowledgeIncludeStateschecks atapi_knowledge_toc.go:45. The difference (403 vs silent clamp) is deliberate and stated in the code comment.?state=approvedalone never triggers the gate, so non-owner callers of the existing endpoint are unaffected. - The consumer query matches the producer:
IngestRetroLessonatsrc/pkg/knowledge/retro_lesson.go:98,107-108writesretro,lesson,review-feedbackandStateDraftforFromReviewFeedbacklessons, sostate=draft&tag=retro,review-feedbackfinds exactly those.filterFactsByAllTagsis an AND over tags, which is what the dashboard count relies on. data.totalinfetchKnowledgeReviewDraftsis served by the handler (api_knowledge.go:203), andviewerIsOwner()already exists inindex.html, so the UI only ever requests the gated list as an owner.go build ./pkg/dashboard/ ./pkg/hivectl/...is clean at074bb31; CI test shards are green.
Refs #14551 is the right link form here — the body names what remains (B1–B6, B10–B12) and this PR is child 4 of that decomposition. The red govulncheck is GO-2026-6860 (#14731), not this diff.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
⏸️ CI is red on the base branch (govulncheck (known vulnerabilities)); holding until it recovers — this is not a failure in this PR, so no fix attempt is counted and no one needs to act here. |
There was a problem hiding this comment.
Looks correct to me; read the handler and its callers at head 3f3b3e7, go build of pkg/dashboard, pkg/hivectl/commands and pkg/knowledge is clean, and CI (incl. dashboard server-tests) is green.
- The owner gate is consistent with the existing opt-in:
knowledgeIncludeStates(src/pkg/dashboard/api_knowledge_toc.go:40-48) checks the sameX-Hive-Role+ verified header and silently clamps; the newknowledgeListStates(api_knowledge.go:215-230) checks viarequireOwnerRole(api_agent_control.go:520-527) and refuses with 403. Refusing rather than clamping for an explicitstate=is the right call — a caller who asked for drafts and got approved facts would misread the result. state=all→ParseLifecycleStates(pkg/knowledge/lifecycle.go:47-48) expands to every state, so it correctly trips the owner check too.- Running
knowledgeListStatesbeforeensureKnowledge()(api_knowledge.go:163-170) means a non-owner asking for drafts gets 403 even when knowledge is disabled, so nothing about state is learnable from the enabled/disabled split. Fine. fetchKnowledgeReviewDrafts()(static/index.html:32464) short-circuits on!viewerIsOwner(), so non-owners never issue the gated request and never see a 403 in the console.- Body matches the diff;
Refs #14551names the remaining B1–B6/B10–B12 work, so the non-closing link is correct.
No defects found.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Why
#14663 stores retro lessons that come from human review feedback as owner-approved drafts tagged
review-feedback. It did not give the owner a way to see or find them. This PR adds that owner visibility (child 4 of the decomposition on #14551: B9 dashboard + CLI, B-AC15, B-AC16, and the non-owner list half of B-AC7).GET /api/knowledgegains astatefilter that lists exactly the given lifecycle states. Any state other thanapprovedneeds a verified owner and gets 403 otherwise, so drafts are never silently returned or leaked. It also gains atagfilter: a fact must carry every tag.hivectl knowledge list --state draft --tag review-feedback(owner only). Neither flag combines with--layer.retro+review-feedbackthrough the owner-gated list, and non-owners never request the count.hivectl.md,api-reference.md,knowledge-lifecycle.md. The last one also corrects a bullet that went stale after 🐛 fix(knowledge): keep draft, deprecated and superseded facts away from agents #14552.Approval still goes through the existing owner-only
hivectl knowledge state <id> approved.The tests fail without this change: API filtering, owner gating and the disappearance after approval; the dashboard line through node; CLI flag wiring and refusal. CI is the verdict. Tests were not run locally.
Refs #14551 (remaining: the GitHub review-feedback source, B1–B3, and the retro lane consuming it, B4–B6, B10–B12 and the B9 log line — blocked by #14584, PR #14608 not yet merged)
— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 requested_by=@Danathar