Skip to content

feat(meetings): let the user edit an agent's minutes - #5740

Merged
kyleseaman merged 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-minutes-editing
Aug 31, 2026
Merged

feat(meetings): let the user edit an agent's minutes#5740
kyleseaman merged 1 commit into
kirodotdev:mainfrom
kaizawa97:pr/meetings-minutes-editing

Conversation

@kaizawa97

@kaizawa97 kaizawa97 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

An agent's minutes were read-only. Correcting one name or number required regenerating the document and discarding every other user correction.

The agent must retain ownership of its generated file, so a user edit is stored separately and wins only when the minutes are read.

Why it matters

Minutes are the durable meeting artifact people keep and share. Correctable minutes must preserve both the agent's latest generated source and the user's intentional edits without allowing either writer to overwrite the other or cross the owner boundary.

What changed (motivation -> approach -> change)

  • Store user edits under the fixed owner-scoped data/edits/<meeting>/<agent> tree, separate from generated outputs.
  • Route filesystem reads/writes and shell access through the existing sensitive owner gate, so neither the edited agent nor a sibling agent can read or overwrite the user's sidecar.
  • Overlay an eligible markdown agent's edit when outputs are collected. One shared editability predicate controls both read and write paths.
  • Derive the stale notice when the agent has regenerated its source since the user saved; the user's edit continues to win until explicitly reverted.
  • Delete the meeting's edit sidecars when the meeting is deleted.
  • Keep the user's text byte-for-byte. Generated model output retains its existing redaction contract.
  • Keep the 3 MiB request-body cap needed for the documented 200,000-character limit.
  • Remove unused PUT content and edits.updated_at response fields and their dangling client references.

UI behavior

  • The panel header stays within the four-action limit: only Chat and Mute remain there.
  • Edit and Revert are separate text actions beside the minutes content.
  • Revert opens the shared accessible confirmation dialog before deleting the edit.
  • A save snapshots the submitted draft and clears local dirty state only if the current draft still matches that snapshot. Typing while a slow save is in flight therefore cannot be erased by the older response.
  • Polling never types over a local draft; mutations invalidate the outputs query so the server remains authoritative.

Security and ownership invariants

  1. The agent remains the sole writer of its generated output.
  2. The user edit is a separate owner-controlled sidecar.
  3. Agent identifiers cannot escape into or resolve the edit storage tree.
  4. Both application routes and agent tools enforce the same owner boundary.
  5. Revert deletes only the sidecar; it never reconstructs or overwrites generated content.

Tests

Backend:

  • meetings minutes and routes: 235 passed;
  • additional owner-gate, deletion-cleanup, and AST checks: 3 passed.

Frontend:

  • meetings/API Vitest: 45 passed;
  • final AgentPanel suite: 35 passed;
  • ConfirmDialog, Modal, and focus behavior: 20 passed.

Static and repository gates:

  • TypeScript and targeted ESLint: pass;
  • i18n key parity: 19/19 locales against origin/main;
  • official Black changed-file gate over the 29-file scope: pass;
  • merge-tree against the current main used for final review: clean.

The slow-save regression test uses a controlled deferred promise. No retries, timing sleeps, enlarged acceptance timeouts, or warning filters were introduced.

Manual verification

Verified in the running dashboard: read-only state, editor, edited state, stale notice, and the Revert confirmation step after the visible Revert action.

Screenshots / video

Read-only minutes with the edit control

Editing the minutes in the panel

Stale notice while the user's edit remains authoritative

Checklist

  • One Conventional Commits commit
  • Deterministic tests cover persistence, ownership, slow-save, confirmation, and stale behavior
  • Documentation and all 19 locale files updated
  • Self-review and focused accessibility review completed
  • No secrets, credentials, or internal references in the diff

no linked issue: this feature was contributed directly without a tracking issue; no open or closed issue covers user-editable agent minutes (nearest, #6289, concerns meeting titles).

@kaizawa97
kaizawa97 requested a review from a team August 25, 2026 01:32
@kaizawa97
kaizawa97 requested a review from a team as a code owner August 25, 2026 01:32
@kaizawa97
kaizawa97 requested a review from patrigao August 25, 2026 01:32
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@bolichen97
bolichen97 enabled auto-merge August 30, 2026 00:01
@iamwhatever iamwhatever added the needs-pr-triage PR scanner: awaiting automated triage label Aug 30, 2026
@NicholasRBowers NicholasRBowers added drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Aug 30, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: Two mechanical CI failures in the PR's own code: (1) Focus Cue Gate — the new minutes-edit <textarea> in website/src/apps/meetings/components/AgentPanel.tsx:284 suppresses the focus outline (outline-none) with no cue in its place; fix is a focus-ring class or focus-visible: utility. (2) Coverage Gate — website/src/apps/meetings/api.ts (baselined at 55.6%) dropped to 52.9% because the new edit-minutes API functions are untested; fix is vitest coverage for the new functions. PR Readiness clears downstream once both are green and the review lanes dispatch.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 30, 2026
@bolichen97
bolichen97 force-pushed the pr/meetings-minutes-editing branch from f5a27f0 to 736d04c Compare August 30, 2026 07:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 30, 2026
@bolichen97
bolichen97 force-pushed the pr/meetings-minutes-editing branch from 736d04c to 35449be Compare August 30, 2026 07:32
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 2fd420e95f4bd34f1f5228329b9e15b485ad8443 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification checks are done. The base tree confirms: no pre-existing edit mechanism in the meetings app, useConfirm/_CREW_SECRET_LEAVES/json_body are all existing mechanisms the change reuses rather than duplicates, and the two findings that survive are small (a zero-consumer response field and one inlined re-derivation). Final review follows.

First-Principles-Verdict: CONCERNS

Every major item is derived and declared; only the DELETE response's reverted field ships with zero consumers, and one path derivation is spelled twice.

What this change ships

Intent: let a user correct an agent's generated meeting minutes without losing the correction to the agent's next rewrite — an ADDITION.

  1. User can edit a markdown agent's minutes in the panel — justified
  2. Revert action behind the shared useConfirm dialog — justified, reuses existing mechanism
  3. "Edited" badge and "agent has written more" stale notice — justified visibility
  4. Edits stored as an owner-only sidecar tree, added to the existing _CREW_SECRET_LEAVES floor — justified, derived boundary
  5. Outputs poll now carries an edits map; edit overlays the output — justified
  6. Meeting delete also removes (and refuses a linked) edits directory — justified rider, declared
  7. Minutes PUT gets a 3 MiB cap via a new json_body(max_bytes=) param — justified, arithmetic derived
  8. reverted field in the DELETE response — zero consumers
  9. New i18n keys in every locale — mandated by the i18n gate
  10. meetings.edit_output / meetings.revert_output audit events — undeclared, but on the existing SEL convention

Watch

reverted (handle_delete_output) is read by nothing: revertOutput in useMeetingSession.ts calls .mutate() and discards the response; only tests assert it (0 consumers counted across website/src). A schema property is surface that must be honored forever.

Subtractions

  • Drop reverted from the DELETE response body; {ok, agent_id} matches what the client uses.
  • In store.delete_meeting, replace the inlined data_dir(root).resolve() / k.AGENT_EDITS_DIR / safe_id + contain + _refuse_linked with a call to agent_edits_dir — the same derivation spelled twice (2 sites in store.py) will diverge.

[FIRST-PRINCIPLES-REVIEWED] 2fd420e

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 2fd420e95f4bd34f1f5228329b9e15b485ad8443 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Sidecar-with-precedence is the right shape for two writers of one document; the one real cost (edit freezes the view) is surfaced, not hidden.

[DESIGN-REVIEWED] 2fd420e

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 2fd420e95f4bd34f1f5228329b9e15b485ad8443 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2fd420e

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 2fd420e95f4bd34f1f5228329b9e15b485ad8443 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have all the evidence I need — the full diff, the ConfirmDialog primitive, the Btn/ui siblings, and the locale catalogs. Emitting the review.

UX-Verdict: CONCERNS

The only way to see the agent's newer text is to permanently destroy your own edit — and the flow never says that loss is unrecoverable.

Watch

  • Stale notice funnels users into irreversible loss. During a live meeting the note-taker rewrites its file every batch, so any edit goes stale within seconds; staleEdit ("…discard your edits to see theirs") offers exactly one action, and revert_agent_edit is a sidecar unlink with no restore. A user who just wants to peek at the new text confirms "Discard my edits" and their correction is gone. High frequency (every mid-meeting edit) × irreversible work loss × every occurrence. Smallest fix: revertHint/confirm body states the edit cannot be recovered ("permanently deletes your version").
  • Open drafts die silently. Cancel discards the draft with no dirty check (setDraft(null) even when draft !== output), and any unmount — leaving the meeting view, an agent toggled off — loses it too; only the chat toggle is guarded. Moderate frequency × loss of a long typed correction. Smallest fix: dirty-check Cancel through the already-imported useConfirm.

Suggestions

  • An empty saved edit renders awaitingOutput ("{{name}} output will appear here.") — false, since the empty edit keeps winning; suppress that string when edit is present.
  • Entering edit mode unmounts the clicked button and drops focus to <body>; autofocus the textarea.
  • While a save blocks on the outputs refetch, Save/Cancel merely grey out; give SendBtn a pending label ("Saving…") so the wait reads as progress, not a dead button.

[UX-REVIEWED] 2fd420e

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 2fd420e95f4bd34f1f5228329b9e15b485ad8443 via the fork AI-review pipeline; updated in place on each push.

Review details

I've read the instruction set, the untrusted candidate list, and the full diff (backend store/routes/security, docs, and frontend).

Falsifying CANDIDATE 1 (stale edit resurfaces after markdown→html→markdown flip):

  • The sidecar filename is <agent>.md; on flip-back to markdown read_agent_edit does re-find it — (a) and (b) hold. But (c) fails. The resurfaced edit is not "silently" winning: _collect_outputsread_agent_edit sets stale = generated_at > edited_at (True here, since the agent kept writing), the frontend renders the staleEdit notice and an "Edited" badge, and once the agent is markdown again the DELETE/revert route works. The design doc and code explicitly document "edit wins, stale tells you, revert is a delete." So there is no observable wrong outcome — the behavior matches the documented contract.
  • The "un-revertable window" while the agent is html: during that state the overlay filters the agent out via _is_editable, so the orphaned .md sidecar is never served and never handed to the iframe renderer (pinned by test_a_sidecar_is_ignored_once_the_agent_turns_into_an_html_widget). An invisible, unserved file is not data loss, corruption, a crash, or a boundary breach; the 409 on DELETE for a non-editable agent is consistent, not a hole.
  • The candidate's own confidence line is "low" and concedes it is "arguably the documented behavior." It does not reach 80+.

Dropped.

Step 2: I checked the load-bearing pieces for a groundable self-finding — the apps/meetings/data/edits sensitive-path registration under both _CREW_HOME_PREFIXES (covered, verified against the hook-gate test), _refuse_linked's lexical-vs-resolve invariant across root/dir/file components, the redaction asymmetry (owner-authored text served to the authenticated owner; only markdown agents editable, rendered through the same MarkdownRenderer path), the surrogate/UTF-8 and body-cap validation, and delete_meeting's edit-dir teardown. None yields a reachable, concrete defect at 80+ on a line this diff changes.

No findings.

[OPUS-REVIEWED] 2fd420e

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@bolichen97
bolichen97 force-pushed the pr/meetings-minutes-editing branch from 35449be to 3fc225c Compare August 30, 2026 08:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 30, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: disposition for span=4b2b2e3acf4c — fixed in 33f087bf1.

  • Unpaired surrogates crash saves with a 500 (origin: validation) (src/kiro_crew/apps/builtins/meetings/backend/routes/meeting_lifecycle.py)

A JSON body can spell an unpaired \ud800 escape, which decodes into a Python str that UTF-8 cannot encode; the failure then happened inside the sidecar write and surfaced as a 500 for what is a malformed client input.

Verified legitimate — the regression test reproduced the exact 500 against the un-fixed route. Fix: handle_put_output validates content.encode("utf-8") after the type/length checks and rejects unpaired surrogates with a 400 (content_not_unicode). Test posts a raw body with a lone \ud800 escape and asserts 400 + code — red (500) before the fix, green after.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@NicholasRBowers
NicholasRBowers force-pushed the pr/meetings-minutes-editing branch from 33f087b to bc92ea4 Compare August 30, 2026 18:58
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: disposition for span=f6914c0df040 — fixed in bc92ea4bd. Span ledger: second hit on this span (round 2: linked sidecar FILE; round 3: linked edits DIRECTORY), so this round replaces point checks with the class-closing invariant rather than patching the instance.

  • Linked edit directories redirect writes into generated minutes (src/kiro_crew/apps/builtins/meetings/backend/store.py)

A pre-existing edits/<id> link to meetings/<id> makes a PUT resolve into the meeting's generated directory, overwriting the generated minutes — the sibling branch of round 2's file-level link.

Verified legitimate: contain() anchors at the whole data dir, so an in-root directory link passes containment. Fix is the invariant that makes the whole family unreachable: new _refuse_linked requires resolve() == lexical spelling (built from the resolved data root + validated components) and is enforced at every component of the sidecar chain — the edits ROOT (agent_edits_root), the per-meeting DIRECTORY (agent_edits_dir), and the sidecar FILE (agent_edit_path) — plus delete_meeting's edits-cleanup path, whose rmtree previously only guarded the final component. The resolve-comparison also catches Windows junctions, which is_symlink() does not. Branch table is in the test docstrings: three regression tests cover root/directory/file links respectively, each asserting the operation is refused and the generated document survives — the two new tests fail against the un-fixed code and pass with it. Local gates: 240 meetings backend tests, isort/flake8/mypy/black all green.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026
An agent's minutes were read-only, so one wrong line could only be
fixed by regenerating the whole document, which discarded every other
correction. Minutes are now editable in place, saved per agent, and
revertible to what the agent produced.

Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
@NicholasRBowers
NicholasRBowers force-pushed the pr/meetings-minutes-editing branch from bc92ea4 to 2fd420e Compare August 30, 2026 20:07
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: disposition for span=53bfb3fec851 — fixed in 2fd420e95. Span ledger: second hit on this span (round 1: refetch left PENDING and void-ed; round 4: refetch FAILED but resolved), so this covers the settle family whole rather than the instance.

  • Failed refetch can discard a saved correction (website/src/apps/meetings/hooks/useMeetingSession.ts)

Successful PUT + failed refetch → invalidation resolves → editor closes → re-saving stale cached text overwrites the correction.

Verified legitimate: invalidateQueries resolves by default even when the underlying refetch errors, so the mutation settled successfully over a pre-save cache. Fix as suggested: both the save and revert mutations now pass { throwOnError: true } as the second invalidateQueries argument, so a failed refetch rejects the mutation — it lands in onError (failure toast) and the editor stays open instead of closing over stale data. Settle-family branch table: refetch pending → mutation held open by returning the promise (round 1 fix, still in place); refetch failed → mutation errors via throwOnError (this fix); refetch succeeded → mutation resolves with the fresh cache (normal path). New regression test "fails the save when the outputs refetch fails" asserts the save rejects and the error toast fires — it fails against the un-fixed hook and passes with it. 62/62 hook coverage tests and tsc -b green locally.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: disposition for the UX Review 🟡 CONCERNS verdict on 2fd420e95accepted-and-deferred to #7116, item by item. Readiness has passed and these are advisory; deferring rather than widening a converged round.

Stale notice funnels users into irreversible loss — the confirm never says the discarded edit is unrecoverable.

Legitimate: revert_agent_edit is a sidecar delete with no restore, and the confirm copy doesn't say so. Deferred to #7116 (add "permanently deletes your version" to the confirm body across locales) — a copy+i18n change is safe to land as a follow-up without re-arming five review lanes on this PR.

Open drafts die silently — Cancel and unmount discard a dirty draft with no check.

Legitimate; the chat toggle is guarded but Cancel/unmount are not. Deferred to #7116 (dirty-check Cancel through the already-imported useConfirm).

The three suggestions (suppress awaitingOutput under a present-but-empty edit, autofocus the textarea on entering edit mode, "Saving…" pending label on SendBtn) are each captured as checklist items in #7116.

@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: disposition for the First Principles Review 🟡 CONCERNS verdict on 2fd420e95accepted-and-deferred to #7116, item by item. The review itself confirms every major item is derived and declared; the two survivors are small and advisory.

reverted (handle_delete_output) is read by nothing — a schema property is surface that must be honored forever.

Legitimate: revertOutput calls .mutate() and discards the response; only tests assert the field. Deferred to #7116 (drop it from the DELETE response, keep {ok, agent_id}) — removing response surface is a clean follow-up and not worth re-arming the full review fleet on a converged head.

The same derivation spelled twice (2 sites in store.py) will divergedelete_meeting inlines the data_dir(...).resolve() / AGENT_EDITS_DIR / safe_id + contain + _refuse_linked chain instead of calling agent_edits_dir.

Legitimate duplication note; both sites currently enforce the identical link-free invariant, so there is no behavioral gap today. Deferred to #7116 (route delete_meeting's cleanup through agent_edits_dir).

@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: PR #5740 is review-ready at head 2fd420e95f4bd34f1f5228329b9e15b485ad8443.

  • PR Readiness: passed — all 70 checks green, both AI review lanes (GPT 5.6, Opus 4.8) clean on this head, 0 unresolved threads, mergeable, single commit authored by Kai Mitsuzawa.
  • 5 fix rounds driven to green, each with a red-before regression test:
    1. Save/revert mutations held pending until the outputs refetch lands (stale-cache overwrite).
    2. Sidecar symlink refusal + byte-for-byte newline round-trip + unpaired-surrogate 400.
    3. Class-closing _refuse_linked invariant across the whole edits chain (root/dir/file/delete) — held against subsequent review rounds.
    4. throwOnError: true so a failed refetch fails the save instead of closing the editor over a stale cache.
  • Every raised concern answered: 6 span-scoped GPT dispositions (no rule violations), Design Review PASS, and the advisory UX + First Principles CONCERNS on this head each dispositioned as accepted-and-deferred to meetings: follow-ups from PR #5740 advisory reviews (minutes editing) #7116.
  • The sidecar-overlay design (agent owns its file; user edit is a separate sidecar that wins on read; stale derived from two mtimes) is unchanged throughout.

Ready for maintainer review. Auto-merge was not armed.

@kyleseaman
kyleseaman disabled auto-merge August 31, 2026 01:27
@kyleseaman
kyleseaman merged commit cae7131 into kirodotdev:main Aug 31, 2026
109 of 111 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 31, 2026
@dwu96 dwu96 removed the drive-to-green PR claimed by drive-to-green pipeline label Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants