Skip to content

/board work step 5d/5e: let a repo-declared expert merge instead of always asking the user to confirm - #804

Merged
CSalcedoDataBI merged 9 commits into
nextfrom
issue-797-board-work-step-5d-5e-let-a-repo
Oct 24, 2026
Merged

CSalcedoDataBI merged 9 commits into
nextfrom
issue-797-board-work-step-5d-5e-let-a-repo

Conversation

@CSalcedoDataBI

Copy link
Copy Markdown
Owner

Closes #797

…#797)

Board-Work.ps1 -MergeAuthority show|expert|user|auto reads or records who
decides the merge. Detection is deterministic and file-only: config
mergeAuthority, or an approved FICHA.md plus .claude/agents/experto.md.
Repos that declare nothing keep the user confirmation flow; the review gate
always runs. A declared expert also makes grouped PRs the default, while a
recorded preferGroupedPRs still wins.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review PR #804 (merge authority por experto, #797)

El diseño es claro y la detección es determinista, con buenos fallbacks (draft de FICHA, experto ausente, valor ilegible). Hallazgos:

  1. Seguridad / alto: el PR puede autoconcederse el merge sin confirmación. Get-WorkingTreeRoot lee FICHA.md y .claude/agents/experto.md del checkout de la rama. Está dicho a propósito, pero significa que el PR bajo revisión puede añadir esos dos archivos, con una línea Aprobada: 2026-01-01 escrita por el propio autor, y pasar de user a expert, saltándose la confirmación humana. Además el "experto" que decide es un archivo que la misma rama puede reescribir. Sugerencia: resolver la autoridad desde la rama base/default (git show origin/<base>:FICHA.md) y no desde el working tree. Como mínimo, no honrar la detección por archivos si el PR modifica FICHA.md o .claude/agents/experto.md, y añadir un test de ese caso.
  2. Medio: "Aprobada" es una aprobación sin autenticar. Cualquier regex-match de fecha cuenta como firma del owner. Considera exigir que la línea venga de la rama base, o al menos que la fecha no sea futura.
  3. Menor: comentario huérfano. El bloque MERGE AUTHORITY quedó insertado entre el comentario de -PreferGroupedPRs y su if. Muévelo junto a su código.
  4. Menor: mergeAuthority: "auto" escrito a mano en config produce el warning "not expert or user; ignored". Trátalo como "sin decisión" sin warning.
  5. Menor: contrato en verbs-work.md. El "merge sin preguntar" depende de que un subagente LLM responda merge. Conviene re-ejecutar -MergeAuthority show justo antes del paso e, no solo antes del d, por si el PR tocó esos archivos entre rondas.

Tests: la cobertura de la unidad es buena (en lo que se ve), pero falta el caso del punto 1 (la autoridad no debe cambiar por archivos que añade el propio PR).

… repos (#797)

An unreadable config.json or an unrecognised mergeAuthority value now reads
as user instead of deferring to the files. -PreferGroupedPRs auto records
the word in a repo that declares an expert, so it beats the expert default.
The FICHA approval regex accepts underscore emphasis, and the step-5d doc
test is anchored on the step-5d block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review PR #804 (merge authority / experto)

🔴 Seguridad: el PR puede autoconcederse el derecho a mergear.
Get-MergeAuthority lee FICHA.md y .claude/agents/experto.md del working tree del checkout actual (Get-WorkingTreeRoot, Board-Work.ps1). El comentario lo presenta como intencional ("a branch may add or change"). Pero una rama que añade FICHA.md con Aprobada: 2026-10-01 y un experto.md pasa de user a expert. Desde ahí el merge se hace sin preguntar al usuario, y el verdicto lo da un agente cuyo prompt controla la misma rama. La "aprobación del owner" no se verifica en ningún punto.
Sugerencia: resolver la autoridad contra la rama base (git show origin/<default>:FICHA.md, o sobre el merge-base). Alternativa: exigir que la declaración esté en config.json ya mergeado en la base. Como mínimo, si esos archivos difieren de la base, degradar a user.

🟡 Aprobación débil. Cualquier línea Aprobada: <fecha> activa la autoridad. No se valida que la fecha no sea futura, ni quién aprobó, ni que el FICHA no cambió después de aprobarse. Conviene documentar que es una convención de confianza, no una verificación.

🟡 Condiciones del merge solo en prosa. Las condiciones de merge (CI verde, sin zona protegida, review en el último commit) se aplican solo mediante verbs-work.md. Ningún código las comprueba antes de Board-Merge.ps1. Considera que Board-Merge.ps1 revalide el gate (exit 0 y review sobre el head sha) en modo expert.

🟢 Menores

  • Tests: faltan casos para -PreferGroupedPRs auto en repo expert (guarda auto en vez de null) y para -MergeAuthority con config.json corrupto al escribir.
  • Get-Content ... catch { "" } traga el error de lectura de FICHA. Falla cerrado, bien, pero un warning ayudaría a diagnosticar.

Lo demás (fail-closed ante valores ilegibles, read-back tras escribir, cobertura de detección) está bien resuelto.

…797)

At step 5d the checkout is the PR under review, so a PR that adds an
approved FICHA.md, loosens experto.md or writes mergeAuthority=expert could
grant itself the merge. Get-MergeAuthority -GitRef reads the declaration as
committed on the default branch; opt-outs still count from anywhere. The
unreadable-config e2e test now uses content that carries no opt-out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review (revisión estática del diff; no ejecuté los tests): no encontré bugs bloqueantes.

  • La resolución de mergeAuthority falla cerrada: valores ilegibles o un config corrupto resuelven a user. Los opt-outs valen desde cualquier fuente, y el grant expert solo cuenta si está commiteado en la rama por defecto. Así un PR no se autoriza a sí mismo.
  • El readback tras escribir la config y la cobertura de tests (319 líneas nuevas en Board-Work.MergeAuthority.Tests.ps1) están bien.
  • Sugerencia menor (no bloqueante): la regex de Aprobada: acepta prefijos como >, * o _, así que una línea citada o de ejemplo en FICHA.md también contaría como aprobación. Sería bueno un test que fije ese comportamiento.

…797)

The contract now loads experto.md and FICHA.md from the same ref as the
grant, and a PR touching the rules of the merge is always propose. An empty
experto.md is no expert in either read mode, and an unparseable committed
config.json gets its own warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review (sha 97cdb98)

Sin bloqueantes. El diseño falla cerrado (valor ilegible, config sin parsear o ref sin resolver dan user), el grant se lee solo de la rama por defecto y el opt-out cuenta desde cualquier fuente. Hay tests dedicados (Board-Work.MergeAuthority.Tests.ps1) y se actualizó GroupedPRsSetting.

Observaciones menores, no bloqueantes:

  • Resolve-IssueBaseRef -NoFetch usa origin/HEAD local, que puede estar desactualizado. Si alguien mergea un FICHA.md o config.json en main y no hay fetch, el grant o el opt-out no se ven hasta el siguiente fetch. Está documentado, pero conviene que el paso 5d haga git fetch antes de -MergeAuthority show, o que show avise de la antigüedad del ref.
  • La regex de Aprobada: acepta >, *, _ y - como prefijo. Una línea citada o dentro de un bloque de código también cuenta como aprobación. Es poco probable que importe, pero merece un test negativo (por ejemplo Aprobada dentro de un fence) si se quiere endurecer.
  • Get-RepoMergeAuthority se evalúa varias veces por ejecución (cada una con varios git show). Funciona, pero se podría cachear.

A committed mergeAuthority array no longer unrolls into a grant or into
"nothing declared". A repo that declares nothing gets the plain default
even when its default branch cannot be resolved. New e2e tests prove
Board-Work reads the declaration from the default branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review PR #804 (merge authority / experto)

Diseño sólido: falla cerrado, el grant se lee de la rama por defecto (no del PR), los opt-outs cuentan desde cualquier fuente, y hay read-back al escribir config. No encontré bugs bloqueantes. Observaciones menores:

  1. Control solo por prompt (mantenibilidad/seguridad): la regla "un PR que toca FICHA.md, experto.md o config.json ⇒ propose" y "el experto lee desde la rama por defecto" viven solo en verbs-work.md; ningún script las impone. Considera un chequeo determinista (p. ej. flag en Board-Work.ps1 que liste si el diff toca esos paths) para no depender de que el modelo lo recuerde antes de un merge irreversible.
  2. Regex de Aprobada: (Get-BoardConfig.ps1) acepta la línea en cualquier parte del FICHA (bloques de código, citas, texto de borrador). Es poco probable, pero anclarla a una sección/encabezado o a frontmatter la haría más difícil de activar por accidente.
  3. Tests: no vi cobertura de que -MergeAuthority auto limpie la clave (null) ni del caso -PreferGroupedPRs auto en repo expert (escribe 'auto' literal); si ya están en el archivo nuevo, ignora este punto.

Por lo demás, OK para mergear tras considerar el punto 1.

…onsistency (#797)

The FICHA approval regex no longer crosses a newline, so a blank Aprobada
field followed by a dated line stays a draft. A lone FICHA.md or experto.md
gets no warning when the default branch cannot be resolved, local opt-outs
are reported before the ref is touched, and warnings name the right source.
The contract says to fetch first and drops its remaining always-confirm
absolutes. Tests cover multi-line files on the ref, origin/HEAD pointing at
master, lone FICHA silence, and reset LASTEXITCODE before each e2e run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review (sha 320aadc): sin bloqueantes. Leí el diff de los scripts, el contrato y la cabecera de los tests. No ejecuté la suite.

Lo bueno: falla cerrado (valores ilegibles, arrays y config sin parsear dan user). El permiso expert solo cuenta si viene de la rama por defecto, y los opt-outs cuentan desde cualquier fuente. La regex de Aprobada: es de una sola línea. Los tests cubren los casos límite.

Observaciones menores, no bloqueantes:

  1. Ref desactualizada. El grant se lee de origin/HEAD según el último fetch. Si alguien olvida el git fetch origin, una revocación remota no cuenta. El contrato ya lo documenta, pero solo depende de que el agente lo haga. Haría el fetch dentro de -MergeAuthority show, o avisaría con la antigüedad del ref.
  2. Aprobación sin verificar. Cualquier fecha con formato Aprobada: YYYY-MM-DD vale, sin comprobar quién la escribió. La protección real es que la FICHA venga de la rama por defecto. Conviene exigir CODEOWNERS o una regla de rama sobre FICHA.md, experto.md y config.json. Así se protege el flujo expert que se salta la confirmación.
  3. Tests de cableado. Uno de ellos hace match de regex sobre el código fuente del script. Es frágil ante cualquier reformateo. Prefiero probar el comportamiento, como ya hacen los casos end-to-end.

AllowUnreviewed under an expert is propose, the judge receives the
default-branch definition and FICHA directly, the pre-expert auto caveat
is stated in step 5, and a missing origin/HEAD has a remedy. Tests pin
lone experto.md silence, an unparseable committed config's warning, and
the posture wiring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Sin hallazgos bloqueantes. La resolución falla cerrada (opt-outs desde cualquier fuente, el grant solo desde la rama por defecto, lectura de config no parseable → user), las dependencias (Test-IsJsonObject, Resolve-IssueBaseRef) existen y la cobertura de tests de MergeAuthority es amplia.

Nota menor (no bloqueante): -MergeAuthority show lee origin/HEAD sin hacer fetch, así que una revocación remota no cuenta hasta que se haga git fetch origin. El contrato ya lo documenta, pero valdría la pena avisarlo también en la salida del script cuando el grant es expert. Ya lo hace en una línea, así que basta con mantenerlo.

#797)

A committed config.json whose root is an array no longer unrolls into a
grant. An empty experto.md is named as empty on both routes, the local-only
warning no longer rides on a files grant, and both read modes treat an empty
file the same. show prints the plan line only when a FICHA exists. The
contract treats a failed fetch as user and re-checks after every fix loop.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Sin hallazgos: la autoridad de merge falla cerrada (config ilegible, valores no reconocidos, arrays raíz y ref no resoluble resuelven a user), el grant se lee solo desde la rama por defecto (no desde el PR), y hay tests dedicados para el resolver y el ajuste de PRs agrupados. No ejecuté los tests; revisé solo el diff.

@CSalcedoDataBI

Copy link
Copy Markdown
Owner Author

External review - claude-review (Claude API, clean context)

Reviewed commit: e8c87ddaf8a41d240d12cc9f2d3ffa1c8e3c95d8

8 rounds at xhigh effort, split per file once the diff outgrew one call. Real defects found and fixed: fail-open on unreadable/garbage/array config; the grant, the expert and FICHA were read from the PR checkout (now the default branch; a PR touching the merge rules is always propose); a FICHA approval regex that crossed newlines; empty experto.md; misleading warnings; doc absolutes; test gaps. Round 8: all four parts patch is correct, zero findings.

@CSalcedoDataBI

Copy link
Copy Markdown
Owner Author

On hold: waiting for the Anthropic plugin directory review. Do not merge yet.

The agentic-board plugin is in its first review in the Anthropic plugin directory, and the directory tracks main. Every merge to main becomes a new version that replaces the one under review and restarts the queue. This PR stays on its branch until the plugin leaves review, or until the owner decides to merge the whole batch at once.

Batch waiting together: #799, #800, #801, #802, #803, #804.

@CSalcedoDataBI CSalcedoDataBI added the on-hold-directory Waiting for the Anthropic plugin directory review before merging label Oct 24, 2026
@CSalcedoDataBI
CSalcedoDataBI changed the base branch from main to next October 24, 2026 13:15
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Revisión del PR #804 (sha 7870bd3): sin hallazgos bloqueantes.

  • La resolución de autoridad de merge falla cerrado: valores ilegibles, config corrupta, ref inexistente y arrays raíz resuelven a user. La concesión expert solo cuenta desde la rama por defecto, no desde el checkout del PR. Buen diseño.
  • Cobertura de tests amplia (528 líneas nuevas en Board-Work.MergeAuthority.Tests.ps1).

Nit menor (opcional): en Board-Work.ps1 el bloque nuevo MERGE AUTHORITY quedó insertado entre el comentario de cabecera de -PreferGroupedPRs y su código, así que ese comentario ahora encabeza el bloque equivocado. Muévelo debajo del bloque nuevo o duplica la cabecera.

@CSalcedoDataBI
CSalcedoDataBI merged commit a96ee71 into next Oct 24, 2026
5 checks passed
@CSalcedoDataBI
CSalcedoDataBI deleted the issue-797-board-work-step-5d-5e-let-a-repo branch October 24, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on-hold-directory Waiting for the Anthropic plugin directory review before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/board work step 5d/5e: let a repo-declared expert merge instead of always asking the user to confirm

1 participant