Skip to content

approvals: sys_approval_request.approval_reject / approval_recall declare both confirmText and params, so one decision opens two sequential dialogs #7278

Closed

Description

@yinlianghui

Out-of-scope finding from objectui#3055 (record page now runs these declared actions). Filed per PD #10 — unassigned, ungraded.

Fact

The shared console action runner chains confirmation then param collection, both awaited, in packages/core/src/actions/ActionRunner.ts of the objectui repo:

  • :794-806 — confirmText → confirmHandler(...), cancel aborts;
  • :809-832 — immediately after, actionParams / a params array → paramCollectionHandler(...), a second dialog.

packages/plugins/plugin-approvals/src/sys-approval-request.object.ts declares both keys on two actions:

action line confirmText params
approval_reject :277-296 "Reject this request? A rejection is final for every approver." comment, attachments
approval_recall :370-384 "Recall this request? Approvers can no longer act on it…" comment

So rejecting is: confirm prompt → Continue → another dialog the approver did not ask for → Confirm. Nothing is sent until the second one.

Why this is the producer's to decide, not the consumer's

objectui#3126 hit exactly this shape on the record page's hand-written reject button and removed confirmText there, keeping the question as the param dialog's description — "the param dialog IS the confirmation: it is titled by this label, carries the confirm question as its description, and nothing is sent until its own Confirm." That reasoning was applied to console code because the buttons were console code.

objectui#3055 retires those hand-written buttons in favour of these declared actions, so the record page now inherits the declared shape — and the approvals inbox has had it all along. The fix belongs in the declaration, not in a consumer special-case: a console that stripped confirmText when params are present would be deciding a metadata author's intent for them, and would do it for every object, not just this one.

Worth noting the two dialogs are not cosmetic on a decision surface: the first one already reads as "the action is running", which is what made objectui#3126's version read as a silent no-op.

Suggested fix (left to triage, not self-selected)

  1. Drop confirmText from both actions and carry the question as the param dialog's description (mirrors objectui#3126's ruling for the same wording on the same decision);
  2. keep confirmText and drop the params from these two (loses the reject comment + attachments — almost certainly wrong);
  3. rule that confirm-then-collect is intended for irreversible decisions and leave both.

Leaning 1: one condition, one wording, one dialog — and the approver is already being asked to confirm, in a dialog titled by the action.

Dedup

Searched open issues in this repo for approval_reject confirmText param dialog: no hits. objectui#3126 is the same question decided for the retired console-side copy, i.e. the precedent, not a duplicate.

Refs: objectui#3055, objectui#3126, objectui#2678.

Activity

  1. os-zhuang commented on Aug 10, 2026

    @os-zhuang
    Contributor

    Triage: pm:queue + domain:services.

    • Class: concrete defect with named declaration sites, and the product question it raises was already answered on the identical wording — objectui#3126 ruled for the hand-written reject button that the param dialog IS the confirmation (question carried as the dialog's description, nothing sent until its own Confirm). This card is the same decision applied at the declaration the retired buttons were replaced by, so it queues as execution of that precedent rather than re-entering the decision box. Suggested direction = the card's option 1 (drop confirmText from approval_reject / approval_recall, carry the question as the param dialog description). If the implementer finds evidence the confirm-then-collect chain was deliberate for irreversible decisions (option 3), stop and escalate instead of shipping — that would contradict the docs(generator): fix reference $ref links and opaque Object types #3126 precedent and needs the maintainer.
    • Routing anchor: both declaration sites verified live on origin/main @ 397e731 — packages/plugins/plugin-approvals/src/sys-approval-request.object.ts:291 (approval_reject confirmText) and :387 (approval_recall confirmText), each alongside params. plugin-approvals ⇒ domain:services. (Body line anchors :277-296/:370-384 have drifted a few lines — same declarations.)
    • Dedup: filer searched and found none; objectui#3126 is precedent, not duplicate. The consumer-side chaining (ActionRunner.ts confirm→params) is objectui code but the fix is producer-side by the card's own argument — no seam, no split needed.
    • target:v17: not applied — a double dialog is a UX defect on a decision surface, not data loss / contract lie / migration shape; doesn't meet the four blocking classes.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. self-assigned this
    on Aug 10, 2026
  3. os-help commented on Aug 10, 2026

    @os-help
    Collaborator

    Claim: PM loop round 3 (domain:services seat #6021)
    Session: session_015fkdTyGmMD5s8ZtEifvuGy
    Branch: claude/issue-7278-approval-confirm-dialogs
    Worktree: objectstack-issue-7278
    Domain: domain:services
    File surface: packages/plugins/plugin-approvals/src/sys-approval-request.object.ts + tests in the same package. (Stop on breach; explain in the report.)
    Container & model: M, mode:subagent, model: opus
    Serial constraints cleared: PRs #7206/#7283 (plugin-approvals/plugin-reports, merged) are in the base; no in-flight claim touches plugin-approvals. Triage re-verified anchors @ 397e731 (:291 / :387).


    Generated by Claude Code

  4. os-help commented on Aug 10, 2026

    @os-help
    Collaborator

    Escalating to the decision box (needs-user-decision); claim released. The dispatched dev returned needs_decision with zero code — correctly. Reviewed by the domain:services seat (#6021, session session_015fkdTyGmMD5s8ZtEifvuGy).

    Background

    The ruled direction (objectui#3126 precedent, option 1: drop confirmText, carry the question as the param dialog's description) is not implementable on the declared surface, measured rather than inferred: objectui's param dialog renders action?.description (ActionParamDialog.tsx:221, set via actionDescription(...) in two independent handlers) — but ActionSchema (packages/spec/src/ui/action.zod.ts, strictObject) refuses a top-level description key, and the action translation node refuses it too (both probed through the real rebuilt schemas; extractor and generated bundles corroborate). So the consumer machinery exists and is dead code: no producer can emit the key. The dev also found NO evidence the confirm-then-collect chain was deliberate (no ADR, no doc, the only nearby comment justifies variant: 'danger'), so the option-3 escape does not apply on its merits.

    The concrete question

    Should description become an authorable key on the public action contract (ActionSchema + actionTranslationSchema + the i18n extractor's emitted set), so the #3126 ruling is executable at the declaration layer?

    Options (three-axis analysis)

    A — Add description to the action contract (spec card), then land #7278 as the two-line move it was scoped to be. RECOMMENDED.

    • 实际业务需求: measured, not asserted — objectui reads action?.description at exactly the dialog line in two handlers, useObjectLabel documents the _actions.{action}.description convention, and ActionParamDialog renders it; all dead today for want of a producer. Plus 20 confirmText+params sites repo-wide (platform-objects: 16 more actions declare both confirmText and params, so one click opens two sequential dialogs (same shape as #7278) #7309; rotate_client_secret opens THREE dialogs) would all be fixed by the same key. This is existing consumer machinery missing its producer half — the startup-focus test is met on evidence.
    • 项目长远合理性: resolves the confirm-vs-collect duplication at the contract layer, where the card's own analysis says it belongs; a consumer-side special case would decide authors' intent for them on every object.
    • 防 AI 写错: declared = enforced restored in mirror image (a consumer read with no legal producer is the same class as a producer with no consumer); once the key exists, a schema-level refuse-or-warn on the confirmText+params PAIR becomes possible — the structural fix.
    • Cost: public-contract surface expansion — new authorable key, authorable-surface anchor, liveness-ledger entry, translation schema + extractor, docs; ~4 packages.

    B — Drop confirmText, accept the generic fallback line. Cheapest; DELETES the finality warning ("A rejection is final for every approver.") from the product with nowhere to live — on the most irreversible decision surface. Worst outcome.

    C — Move the question into the comment param's helpText (a declared slot, measured accepting). One dialog, no words lost — but a whole-decision warning becomes per-field typing guidance: a #5240 placement distortion, and it teaches authors to smuggle decision-level prose into a slot no gate can see.

    D — Leave as-is (two dialogs stay). Listed only because it changes nothing while the slot question is open; no evidence supports the chain being deliberate.

    Recommendation

    A, split: (i) a spec-lane card for the description key (maintainer-ruled — it expands the public action contract); (ii) #7278 stays open, Blocked-by: that card, then lands as the scoped two-action move; (iii) #7309 (the other 18 sites) follows the same ruling. B/C would quietly re-decide the wording question #3126 already answered.

    Related: #7309 (repo-wide census of the pair, filed by the dev, blocked-by this), objectui#3126 (precedent), #7254 (unrelated doc gap, same "the table doesn't describe the real surface" family).


    Generated by Claude Code

  5. removed their assignment
    on Aug 10, 2026
  6. os-zhuang commented on Aug 10, 2026

    @os-zhuang
    Contributor

    Maintainer ruling (2026-08-10, directed in session session_01BPWqbmEFU8gJepBJTHESXd): Option 1.

    Drop confirmText from approval_reject and approval_recall; the param dialog carries the confirm question as its description — one condition, one wording, one dialog, nothing sent until its own Confirm. This applies objectui#3126's ruling (same wording, same decision) to the declaration side, where it fixes every consumer at once. Option 2 loses the reject comment/attachments; option 3 keeps a first dialog that reads as "the action ran".

    needs-user-decision → pm:queue.


    Generated by Claude Code

  7. 5 remaining items

  8. self-assigned this
    on Aug 11, 2026
  9. os-help commented on Aug 11, 2026

    @os-help
    Collaborator

    Claim: PM loop, services lane, wave 2 round 1 (re-claim after the #7367 unlock; the prior claim was released at escalation)
    Session: session_015fkdTyGmMD5s8ZtEifvuGy
    Branch: claude/issue-7278-approval-confirm-dialogs
    Worktree: objectstack-issue-7278
    Domain: domain:services
    File surface: packages/plugins/plugin-approvals/src/sys-approval-request.object.ts (the approval_reject / approval_recall declarations) + tests in the same package. ⛔ Not packages/spec (the description key landed as #7367 / PR #7430), ⛔ not InlineActionSchema (deliberately excluded by that PR). Stop on breach.
    Container & model: S-grade metadata move with the ruling and the mechanism both in place, mode:subagent, model: opus — the known trap (two same-named description keys on the action shape) is a misuse-of-surface risk, which is judgment, not mechanics.
    Serial constraints cleared: no open PR or in-flight claim touches plugin-approvals — this lane's #7206/#7283 (same package family) merged 08-10 and are in the base. ⚠️ #7527 (approvals REST filter) is queued in the same package and is deliberately NOT in this batch; it rides a later round, serialized behind this card.


    Generated by Claude Code

  10. os-help commented on Aug 11, 2026

    @os-help
    Collaborator

    <!-- os-dev-report -->

    {
      "issue": 7278,
      "status": "done",
      "branch": "claude/issue-7278-approval-confirm-dialogs",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/7592",
      "premise_still_valid": true,
      "summary": "Premise re-verified on origin/main @ 97b66583e: both confirmText lines were still present (:291 approval_reject, :387 approval_recall), each alongside params, and the #7367 mechanism is in place (actionObject() top-level description, docblock naming this card). Executed the maintainer's Option-1 ruling: dropped confirmText from both actions and carried each question verbatim into the action's top-level description, finality warning included. Used the TOP-LEVEL key, not ActionAiSchema.description — a test pins that the question did not land in the AI block. One non-obvious half the card did not anticipate: the four generated locale bundles carried confirmText for both actions, and regenerating them seeded the new description leaf with ENGLISH in zh-CN/ja-JP/es-ES (a renamed key reads to the extractor as a new gap, and --fill=default fills from source), silently discarding the curated translations of the identical sentence; I carried them across by hand and added a pin, because check:i18n structurally cannot see that loss — English in a non-English locale is exactly what a fresh extract produces, so the bundle is 'in sync' either way.",
      "tests": "pnpm --filter @objectstack/plugin-approvals typecheck — clean. pnpm --filter @objectstack/plugin-approvals test — 'Test Files 21 passed (21) / Tests 458 passed (458)' (was 20/452). node scripts/check-i18n-bundles.mjs --filter=plugin-approvals — 'plugins/plugin-approvals in sync (4 bundle(s))', confirming the hand-carried translations survive merge mode. i18n coverage ratchet measured per-config (the whole-gate run cannot complete locally without a full pnpm build — it fails on unbuilt example deps, its own stated fix): 'i18n rule issues: 0', exactly this config's committed baseline of 0. check:docs-audit-scope green (56 + 22 self-test cases, scope in sync). check:nul-bytes green (7016 files). REVERSE VERIFICATION, direction predicted before running: restoring the old declaration via git checkout origin/main -- <object file> turned exactly the two change-detecting pins red — '× the confirm question rides `description`, not `confirmText`…' and '× no declared action pairs `confirmText` with `params`…', 'Test Files 1 failed | 20 passed (21)'. The ai.description guard and the three translation pins stayed green under that revert, correctly: they are non-regression guards, not change detectors. The pre-existing expect(byName('approval_recall').confirmText).toBeTruthy() went red on the fix itself and was replaced by content pins on the wording. CI at report time (recorded honestly, not polled): ESLint in_progress, TypeScript Type Check queued, Check Changeset in_progress — nothing had concluded yet.",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Notes for the PM, outside the JSON:


    Generated by Claude Code


    Generated by Claude Code

  11. os-help commented on Aug 11, 2026

    @os-help
    Collaborator

    ACCEPT — PR #7592, reviewed against GitHub rather than the report's own claims.

    What shipped. The maintainer's Option-1 ruling executed: confirmText dropped from approval_reject / approval_recall, each question carried verbatim into the action's top-level description (#7367's key) — including the finality warning whose loss disqualified option B. One decision, one dialog, nothing sent until its own Confirm.

    Verified myself (get_files, 8 files):

    • The right description. The question sits on the top-level action key; a dedicated test pins ai?.description === undefined for both actions, so the same-named LLM-facing key one level down cannot quietly absorb it later. The in-code comment at the declaration explains the distinction for the next author.
    • Wording pinned by content, not shape — exact-string pins for both questions, plus the two key phrases.
    • An object-wide guard beyond the ask: a new pin asserts no declared action on this object pairs confirmText with a non-empty params, with a failure message that teaches the fix. That is platform-objects: 16 more actions declare both confirmText and params, so one click opens two sequential dialogs (same shape as #7278) #7309's shape held at zero for this object before platform-objects: 16 more actions declare both confirmText and params, so one click opens two sequential dialogs (same shape as #7278) #7309 even runs.
    • The i18n half is the find of this card. The dispatch did not anticipate it: renaming the key made os i18n extract --fill=default seed the new leaf with English in zh-CN / ja-JP / es-ES, silently discarding curated translations of the identical sentence — and check:i18n structurally cannot see the loss (English-in-a-non-English-locale IS what a fresh extract produces, so the bundle is "in sync" either way). The dev hand-carried all three translations (visible verbatim in the diff) and added a 74-line carryover test that turns exactly that silent loss into a red: each locale's question must differ from the English source. check-i18n-bundles confirms the hand-carried strings survive merge mode.
    • Reverse verification in the predicted direction: reverting the declaration reds exactly the two change-detecting pins (1 failed | 20 passed); the ai.description guard and translation pins stay green under the revert — correctly, as non-regression guards. The pre-existing confirmText).toBeTruthy() assertion went red on the fix and was replaced by the content pins.
    • Scope held: no packages/spec, no InlineActionSchema, none of platform-objects: 16 more actions declare both confirmText and params, so one click opens two sequential dialogs (same shape as #7278) #7309's 18 sites, no content/docs/releases/, changeset present (@objectstack/plugin-approvals patch).

    Out-of-scope observation worth one line for triage / #7309's dispatcher: the --fill=default translation-clobber mechanism is generic — every one of #7309's 18 remaining confirmText→description moves will hit it in all non-English locales. The carryover test pattern here is the antidote; #7309's dispatch prompt should mandate it per site.

    CI at review time: gates in progress. Driving to landing: gate conclusions → ready → arm → queue-branch verify.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions