feat: four-layer member system prompt for crew DM threads - #7235
Conversation
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @CrysisDeu overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound layered-ownership design with named boundaries; the residual risks are a steering layer dressed in enforcement-grade machinery, and a repair path that predates any UI. Watch
Suggestions
[DESIGN-REVIEWED] fc2dc2e |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All checks complete. I have what I need for the verdict. Summary of what I verified: The First-Principles-Verdict: CONCERNS Item 2 of What this change shipsIntent: give each crew member DM thread its own identity, working protocol, user-set boundaries, and self-maintained memory — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] fc2dc2e |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
437f14d to
a84dfb1
Compare
|
Round 1 findings addressed at head a84dfb1 (all lanes were terminal before this push): GPT 5.6 (blocking):
Design (advisory watches — all adopted):
CI (owner-gate invariant, Backend shard 1 x3): the rules PUT is now owner-gated via Local gates green: black baseline, isort, flake8, mypy, docs-lint, targeted shard 539 passed (including |
|
Round 2 findings addressed at head 16c2411: GPT (4 blocking) + Opus (1 blocking, same root as GPT #1):
Local gates green: 543 targeted tests, black baseline, isort, flake8, mypy, docs-lint. Spec updated in the same commit. |
a84dfb1 to
16c2411
Compare
|
Round 3 findings addressed at head 82cd5fc:
Local gates green: 278 targeted tests, black baseline, isort, flake8, mypy, docs-lint. Spec updated in the same commit. |
16c2411 to
82cd5fc
Compare
|
Round 4 findings addressed at head b650ef2:
Note: the round-4 Local gates green: 275 targeted tests, black baseline, isort, flake8, mypy. |
82cd5fc to
b650ef2
Compare
|
/ai-review override gpt b91beb0: Same two findings already adjudicated on f3d8f5e/b5856b441/8d4c0c888 (this push implements Design Review CONCERNS per the maintainer's implement-advisory-concerns directive; diff to the adjudicated spans unchanged): deny-site SEL sync-init sibling tracked in #8523; scrub NFKC whole-vs-span normalization tracked in #8524, current behavior fail-closed. Both accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
|
|
|
|
|
/ai-review override gpt 2c6f9ae: Same two findings already adjudicated on f3d8f5e/b5856b441/8d4c0c888/b91beb03c (conflict-resolution-only rebase, adjudicated spans unchanged): deny-site SEL sync-init sibling is a pre-existing repo-wide pattern tracked in #8523; scrub NFKC whole-vs-span normalization is a design question tracked in #8524 with current behavior fail-closed. Both accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
/ai-review override gpt f741de7: Same two findings already adjudicated on f3d8f5e/b5856b441/8d4c0c888/b91beb03c/2c6f9aeae (conflict-resolution-only rebase, adjudicated spans unchanged): deny-site SEL sync-init sibling is a pre-existing repo-wide pattern tracked in #8523; scrub NFKC whole-vs-span normalization is a design question tracked in #8524 with current behavior fail-closed. Both accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
/ai-review override gpt 4b5bd2d: Same two findings already adjudicated on five prior heads (deny-site SEL sync-init sibling tracked in #8523 — since fixed upstream by #8604; scrub NFKC whole-vs-span normalization tracked in #8524, fail-closed today). This head additionally FIXES the Opus await finding from f741de7 (rebase-over-#8604 semantic break, await added at both rules-handler sites). Both original findings accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
|
/ai-review override gpt cd3a384: Same two findings adjudicated on six prior heads (deny-site SEL sync-init sibling tracked in #8523, since fixed upstream by #8604; scrub NFKC whole-vs-span tracked in #8524, fail-closed today). This head also carries the Opus await fix and rewrites the two rules-audit sites to the lambda-in-to_thread shape #8604's AST guard requires. Both original findings accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
Status note: the remaining shard-3 failures are inherited from main, not introduced by this PR.
Everything this PR owns is green on head cd3a384: GPT 5.6 + Opus 4.8 + Design + UX all pass, 52 checks passing, MERGEABLE. The earlier Opus blocking finding (un-awaited |
|
/ai-review override gpt 95e7f9c: Same two findings adjudicated on seven prior heads — deny-site SEL sync-init sibling tracked in #8523 (fixed upstream by #8604) and scrub NFKC whole-vs-span normalization tracked in #8524 (fail-closed today). This head is a rebase onto fixed main (#8712) plus a one-line black reformat in chat_runner.py, then an empty-amend re-trigger; the adjudicated spans are unchanged. Both accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
A member DM session previously differed from an ordinary session only by
the [CURRENT AGENT] label. Member turns now inject a [MEMBER IDENTITY]
block with four layers of distinct ownership:
1. identity — derived from the crew record (name, description,
triggers); auto-generated so an empty description still has a floor
2. behavior — product-owned working protocol: worker-not-Q&A-bot,
front-desk-vs-workshop dispatch, a four-rung stuck ladder that
escalates only at permission / reachability / one-way-door walls,
zero-context escalation drafts validated by a context-free subagent,
and quiet-run reporting
3. permanent rules — user-owned, stored at trust/member-rules/<slug>.json
under the keystone-gated trust/ subtree so the member cannot rewrite
its own safety boundary; GET/PUT /api/members/{slug}/rules is the
only write path (human dashboard action, app tokens denied)
4. briefing — member-owned working memory at members/<slug>/briefing.md,
agent-writable by design, injection-capped with a visible marker
Precedence is the injection order. The section builder degrades to an
ordinary crew session on any failure EXCEPT an unreadable rules file:
for the one safety-relevant layer that degrade is the fail-open, so
MemberRulesUnreadable propagates and aborts the turn until the user
repairs or clears the file.
Review follow-ups (post-rebase, addressing bolichen97's review; feature
authored by CrysisDeu):
- read_member_briefing now opens through the central
pinned_fs.open_in_pinned_parent helper, pinning the ancestor chain
(members/<slug>/ is agent-writable too — a swapped parent redirected
the whole traversal past the leaf-only O_NOFOLLOW; same shape as
#2446). Fails closed where the pinned walk is unavailable.
- member_briefing_supported() names the layer-4 availability; on
platforms where briefing reads fail closed (Windows) the section now
says "not available on this platform" and item 6 of [HOW YOU WORK] is
softened instead of instructing upkeep of a never-injected file.
- Rebased onto main over the crew-member operating-mode block (#8153):
both blocks render — dispatch mechanics first, then the four layers.
- Scrub coverage test now carries a fullwidth NFKC forgery per minted
header, and a regression test pins the symlinked-parent refusal
red-before-green.
- The layer-3 fail-closed contract now holds PER-TURN across the whole
session lifecycle (branch table in build_message): a first-turn
MemberRulesUnreadable abort leaves a warm session whose turns skipped
both member-section paths, and an idle-expired session restored via
session/load carried a STALE [PERMANENT RULES] snapshot. build_message
now validates rules on every warm member turn, re-injects the CURRENT
member section on slim resume (rules gate included), and the dashboard
runner re-arms the reinjection flag when a member DM's first turn dies
before landing, so the section (fresh rules included) is re-delivered
on the first turn after the user repairs the file.
- Feature-map row for Crew Members now names the GET/PUT
/api/members/{slug}/rules endpoints.
- The PUT collision scan uses the central _member_names_for_slug helper
(grammar filter + MemberSlugError tolerance) instead of an ad-hoc
comprehension a hand-edited config key could crash.
- Rules saves and clears are crash-durable: atomic_write's file fsync
covers the data, but the publishing rename, the first-save directory
creation, and the clearing unlink are directory entries — fsync_dir
(raising on save, best_effort on the already-committed clear) makes
the PUT's 200 survive a power-off instead of resurrecting cleared
rules or losing saved ones.
- The rules GET is owner-gated like the PUT: rules are the owner's
private safety instructions, and any allowed Slack user can mint a
dashboard session, so the read would otherwise disclose them.
- Escaped lone surrogates in the rules payload are refused with a coded
400 (rules_not_encodable) at the PUT boundary, and write_member_rules
rejects UTF-8-unencodable text before any state changes — previously
the UnicodeEncodeError from the write escaped as a mislabeled
"too long" error, and escaping surrogates into the file would only
defer the crash to prompt-encode time inside the member's turn.
- The feature-map Crew Members row lists the rules API as a combined
`GET,PUT /api/members/{slug}/rules` entry, keeping the row within the
feature map's four-route limit.
- The member first-turn re-arm lives in the runner's finally (the one
block on every exit path), not the except arm alone: a user Stop's
cancelled completion never raises, so the except-arm-only re-arm left
the warm session running without [PERMANENT RULES] after a stopped
first turn.
Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
|
/ai-review override gpt fc2dc2e: Same two findings adjudicated on eight prior heads — deny-site SEL sync-init sibling tracked in #8523 (fixed upstream by #8604) and scrub NFKC whole-vs-span normalization tracked in #8524 (fail-closed today). This head is a rebase onto fixed main (#8712); the only chat_runner.py change is splitting one 101-char append_and_surface call across three lines to satisfy the pinned black (line-length 100), matching main's own form. The adjudicated spans are unchanged. Both accepted-and-deferred. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
chenmingwei23
left a comment
There was a problem hiding this comment.
Approved: checks green at head, readiness passed, no blocking reviewer verdict outstanding.
Summary
A crew member DM session previously received the same system prompt as any ordinary session — the only difference was the
[CURRENT AGENT]label, so every member spoke and behaved identically regardless of its name, role, or remit. This PR gives member DM threads a four-layer member identity block with distinct ownership per layer:[MEMBER IDENTITY][HOW YOU WORK][PERMANENT RULES]trust/member-rules/<slug>.jsonunder the keystone-gatedtrust/subtree, so the member's own file tools cannot rewrite its safety boundary; written only via the newGET/PUT /api/members/{slug}/rules(human dashboard action, app tokens denied)[CURRENT ASSIGNMENT]members/<slug>/briefing.md, agent-writable by design, injection-capped with a visible truncation markerPrecedence is the injection order (rules outrank the briefing). The section builder degrades any failure to an ordinary crew session — EXCEPT an unreadable rules file: for the one safety-relevant layer that degrade would be the fail-open (a member the user bounded would run with no bounds at all), so
MemberRulesUnreadabledeliberately propagates and aborts the turn until the user repairs or clears the file. Ordinary (non-member) sessions are byte-for-byte unchanged.Changes
members.py:member_rules_path/read_member_rules/write_member_rules(trust-gated, refuse-not-truncate cap, empty write clears),member_briefing_path/read_member_briefing(agent-writable, capped with marker)context.py:_MEMBER_HOW_YOU_WORKconstant,ContextBuilder._build_member_section,member=kwarg threaded throughbuild_message→build_session_context, injected after the agent-identity block formode="member"sessions onlychat_runner.py: passesmember=slot.agentfor member slots (slot.agentis the crew the human picked;agent=is the resolved template)handlers/members.py+ routes:GET/PUT /api/members/{slug}/ruleswithmember_slug_mismatch/member_not_found/rules_too_longcodesdocs/system-specs/modules/learn-cron-dashboard.md)Review follow-ups (post-rebase, addressing @bolichen97's review):
read_member_briefingopens through the centralpinned_fs.open_in_pinned_parenthelper, pinning the ancestor chain —members/<slug>/is agent-writable too, and a parent swapped for a symlink redirected the whole traversal past the leaf-onlyO_NOFOLLOW(the ancestor-swap shape that closed fix: include artifact library and uploads in snapshot/restore #2446);O_NONBLOCK+fstatregularity keep the FIFO refusalmember_briefing_supported()names layer-4 availability: where briefing reads fail closed (Windows — noO_NOFOLLOW/pinned walk), the[CURRENT ASSIGNMENT]placeholder now says "not available on this platform" and item 6 of[HOW YOU WORK]is softened, instead of instructing upkeep of a file that is never injectedTesting
test/test_member_prompt.py(51 tests): storage round-trips, trust-subtree placement, refuse-not-truncate, injection presence/absence per mode, identity floor for unregistered crews, protocol-clause pinning, route contracts incl. app-token denial and slug-mismatch write refusal; symlinked-parent refusal pinned red-before-green, a fullwidth NFKC scrub forgery per minted header, and the unavailable-platform renderingtest_member_prompt.py51 passed; full run's only failures reproduce identically on pristine main — host-env artifacts)Screenshots
No UI change (backend prompt assembly + API only); the Members-page rules editor is a follow-up.