fix(maintainer): mechanical fixes from 2026-07-16 maintainer-workflows review (B1/B2/B3/B5/R1/R4/R2)#2147
Open
Wirasm wants to merge 6 commits into
Open
fix(maintainer): mechanical fixes from 2026-07-16 maintainer-workflows review (B1/B2/B3/B5/R1/R4/R2)#2147Wirasm wants to merge 6 commits into
Wirasm wants to merge 6 commits into
Conversation
added 6 commits
July 16, 2026 13:50
The Claude SDK renamed the sub-agent tool from `Task` to `Agent`; the old
name is silently ignored at runtime. Four nodes in repo-triage.yaml
(triage-issues, closed-dedup-check, closed-pr-dedup-check, link-prs)
whitelisted `Task` in allowed_tools and orchestrate `agents:` fan-out via
that tool, so their brief-gen / matcher sub-agent fan-out was silently
dead and the main agent was improvising the work inline.
Change `Task` -> `Agent` in all four allowed_tools arrays and update the
prompt wording that instructs the model to fan out via the tool. The
Pi/Minimax twin has no allowed_tools Task entry (it does everything
inline and never uses the tool); its prose references are updated to the
new tool name for consistency.
The validator warning ("Tool 'Task' was renamed to 'Agent'") now clears
for both workflows.
…ew command The direction/scope gate phase was removed from maintainer-review-pr (its description now says that path is gone), and nothing writes $ARTIFACTS_DIR/gate-decision.md. The code-review command still opened Phase 1 by `cat`-ing that missing file and framed its scope as "within the scope the gate accepted", so every aspect run began with a missing-file error and stale framing. Delete the "Read the gate decision" section and reword the header line's "for every PR that passes the gate" to "for every PR under review" — the last dangling gate reference in the file.
extract-pr-number was an AI node whose only job was picking a digit run out of $ARGUMENTS, with a bash grep fallback in fetch-pr immediately after — an AI call for something bash does deterministically and for free. Invert to bash-first and delete the AI node. The new bash node reads $ARGUMENTS (injected as a subprocess env var, so quoting is injection-safe), writes the number to .pr-number, and fails loudly when none is present. fetch-pr now just reads .pr-number (like fetch-diff already does), which removes the `"$extract-pr-number.output"` double-quoted-substitution the validator flagged (B5) — the offending reference no longer exists. Extraction anchors on a keyword/URL segment (`pull/`, `issues/`, `#`, `PR`) before grabbing digits, with a bare-number fallback. A naive `grep -oE '[0-9]+' | head -1` on a PR URL would pick the FIRST number anywhere in the string — wrong for this repo, whose org slug `coleam00` makes `...//pull/1428` resolve to "00". The anchored grep returns 1428 for bare numbers, `#N`, `PR N`, and full PR URLs alike.
maintainer-review-pr declares mutates_checkout: false, which permits concurrent runs, yet record-review did a non-atomic read-modify-write of the shared .archon/maintainer-standup/reviewed-prs.json — two overlapping reviews could observe a half-written file or clobber each other mid-write. Write to a temp file in the same directory and renameSync over the target. Rename is atomic on POSIX, so a concurrent reader always sees a complete file. Last-writer-wins is accepted (documented inline): the standup only needs a recent-enough "reviewed" marker.
… (B3)
marketplace-validate-schema emits { valid: true, files: [] } when the
pinned SHA yields no workflow YAML (no source dir / no YAML / no
workflow-shaped YAML with a top-level `nodes:`). The decide node's schema
guard only fired on `valid: false` with a NON-empty file list, so an
empty or wrong-path submission passed schema, scanned clean (nothing to
scan), and reached the auto-merge branch — a fail-open where a marketplace
entry pointing at zero workflows could auto-merge.
Add a deterministic branch in decide: an empty `schemaResult.files` list
(regardless of `valid`) => request_changes with an explicit reason. Placed
after the scope/draft/scan-critical checks and before auto-merge. Document
the empty-list contract in the script's docstring and at each empty exit.
decide is an inline-YAML script node with no unit-test harness, but the
validate-schema script it depends on now has one: a new test pins the
three empty-source cases to files:[] plus non-empty contrast cases.
GOVERNANCE BEHAVIOR CHANGE — needs maintainer sign-off. The marketplace reviewer auto-closed a contributor's PR on any `reject` decision, including an AI-only reject (Haiku recommended reject while the deterministic scope check and security scan were both clean). Closing a human's PR on a single Haiku opinion is the sharpest auto-action in the set (R2). decide now emits a `close: boolean`. It is true only when a DETERMINISTIC signal justified the reject — a critical/high security-scan severity, or a scope violation — and false for an AI-only reject. The act node gates `gh pr close` on it: deterministic rejects auto-close exactly as before; AI-only rejects still post the request-changes review but stay open with a "flagged for maintainer close" comment. Only the AI-only-reject path changes; scope-violation and high-severity scan rejects close identically to before. Everything else is unchanged.
📝 WalkthroughWalkthroughThe PR hardens marketplace review decisions, makes maintainer review execution deterministic and atomic, updates review scope instructions, and replaces Task terminology with Agent terminology across triage workflows. ChangesMarketplace review safeguards
Maintainer review execution
Triage Agent terminology
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SchemaValidator
participant DecideNode
participant ActNode
SchemaValidator->>DecideNode: Return schemaResult
DecideNode->>ActNode: Provide decision and close
ActNode->>ActNode: Request changes and optionally close the PR
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.archon/workflows/maintainer/maintainer-review-pr.yaml (1)
33-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrevent pipeline crashes under
set -eo pipefailand avoidechoflag consumption.If the workflow runner executes this script with
set -o pipefail, thegreppipeline will fail when the prefix doesn't match (e.g. input is just1428), crashing the script immediately and preventing the fallback from running. Appending|| trueensures the pipeline always exits0, allowing the empty-check fallback to execute safely.Additionally, using
printf '%s\n'instead ofechoprevents$ARGUMENTSfrom being inadvertently consumed as a command-line flag if it happens to start with-nor-e.🛡️ Proposed defensive refactor
- PR_NUM=$(echo "$ARGUMENTS" | grep -oiE '(pull/|issues/|#|pr[[:space:]#]*)[0-9]+' | grep -oE '[0-9]+' | head -1) + PR_NUM=$(printf '%s\n' "$ARGUMENTS" | grep -oiE '(pull/|issues/|#|pr[[:space:]#]*)[0-9]+' | grep -oE '[0-9]+' | head -n 1 || true) if [ -z "$PR_NUM" ]; then - PR_NUM=$(echo "$ARGUMENTS" | tr -d "'\"\`\n " | grep -oE '[0-9]+' | head -1) + PR_NUM=$(printf '%s\n' "$ARGUMENTS" | tr -d "'\"\`\n " | grep -oE '[0-9]+' | head -n 1 || true) fi if [ -z "$PR_NUM" ]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/workflows/maintainer/maintainer-review-pr.yaml around lines 33 - 63, Update the PR-number extraction pipelines in the bash step to use printf '%s\n' "$ARGUMENTS" instead of echo, and make the anchored grep pipeline tolerate no-match failures under pipefail (for example by appending || true) so the bare-number fallback executes. Preserve the existing anchored-first extraction and error handling behavior..archon/scripts/__tests__/marketplace-validate-schema.test.ts (1)
26-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the temporary directory after execution.
The
runValidatorfunction creates a temporary directory in the system's/tmpfolder but does not remove it. This can lead to/tmpbloat during frequent local test runs. Wrapping the execution in atry...finallyblock to ensurermSynccleans up the directory is recommended.♻️ Proposed refactor to add cleanup
Ensure
rmSyncis imported at the top of the file:import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';Then apply the cleanup in
runValidator:function runValidator(sourceFiles: Record<string, string> | null): ValidateOutput { const artifactsDir = mkdtempSync(join(tmpdir(), 'validate-schema-')); - if (sourceFiles) { - const sourceDir = join(artifactsDir, 'source'); - mkdirSync(sourceDir, { recursive: true }); - for (const [rel, content] of Object.entries(sourceFiles)) { - const full = join(sourceDir, rel); - mkdirSync(dirname(full), { recursive: true }); - writeFileSync(full, content); - } - } - const output = execFileSync('bun', [VALIDATOR], { - env: { ...process.env, ARTIFACTS_DIR: artifactsDir }, - stdio: ['ignore', 'pipe', 'pipe'], - }).toString(); - return JSON.parse(output) as ValidateOutput; + try { + if (sourceFiles) { + const sourceDir = join(artifactsDir, 'source'); + mkdirSync(sourceDir, { recursive: true }); + for (const [rel, content] of Object.entries(sourceFiles)) { + const full = join(sourceDir, rel); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, content); + } + } + const output = execFileSync('bun', [VALIDATOR], { + env: { ...process.env, ARTIFACTS_DIR: artifactsDir }, + stdio: ['ignore', 'pipe', 'pipe'], + }).toString(); + return JSON.parse(output) as ValidateOutput; + } finally { + rmSync(artifactsDir, { recursive: true, force: true }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.archon/scripts/__tests__/marketplace-validate-schema.test.ts around lines 26 - 42, Update runValidator to wrap the validator execution and JSON parsing in a try/finally block, and call rmSync on artifactsDir in the finally clause with recursive cleanup enabled. Import rmSync from node:fs while preserving the existing temporary-directory setup and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.archon/scripts/__tests__/marketplace-validate-schema.test.ts:
- Around line 26-42: Update runValidator to wrap the validator execution and
JSON parsing in a try/finally block, and call rmSync on artifactsDir in the
finally clause with recursive cleanup enabled. Import rmSync from node:fs while
preserving the existing temporary-directory setup and return behavior.
In @.archon/workflows/maintainer/maintainer-review-pr.yaml:
- Around line 33-63: Update the PR-number extraction pipelines in the bash step
to use printf '%s\n' "$ARGUMENTS" instead of echo, and make the anchored grep
pipeline tolerate no-match failures under pipefail (for example by appending ||
true) so the bare-number fallback executes. Preserve the existing anchored-first
extraction and error handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d8b5e97-59a7-46c0-8234-a24a6b33c328
📒 Files selected for processing (7)
.archon/commands/maintainer-review-code-review.md.archon/scripts/__tests__/marketplace-validate-schema.test.ts.archon/scripts/marketplace-validate-schema.ts.archon/workflows/maintainer/maintainer-review-pr.yaml.archon/workflows/maintainer/marketplace-pr-review-and-merge.yaml.archon/workflows/maintainer/repo-triage-minimax.yaml.archon/workflows/maintainer/repo-triage.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mechanical fixes from the 2026-07-16 maintainer-workflows review (
.claude/archon/reviews/2026-07-16-maintainer-workflows-review.md). One commit per finding: B1, B2, B5, R4, R1, B3, R2..archon/workflows/defaultsorcommands/defaults), no engine/package source touched, no DB schema. B4/R3 (the repo-triage fork consolidation) are explicitly out of scope here.Findings addressed
rename Task tool to Agentdrop removed gate-decision referencesbash-first PR-number extractionatomic write of reviewed-prs.jsonreject submissions with no workflow YAMLdon't auto-close AI-only marketplace rejectsVerification-time discrepancies vs. the review/instructions (flagged for the record)
triage-issues+closed-dedup-check(2 nodes × 2 files = "4 sites"). The validator actually flags four nodes inrepo-triage.yaml(triage-issues,closed-dedup-check,closed-pr-dedup-check,link-prs) — all four whitelistTaskand orchestrateagents:fan-out through it. Since the acceptance criterion is "the warning disappears," all four are fixed.repo-triage-minimax.yamlis a Pi/Minimax fork withallowed_tools: [Bash, Read, Write](noTask) that does all briefing inline and never uses the tool — it validates clean. There were noallowed_toolssites to change there; its prose references to the tool were renamed toAgentfor consistency only.grep -oE '[0-9]+' | head -1ongithub.com/coleam00/Archon/pull/1428returns00(the org slugcoleam00contains digits). The new extractor anchors onpull/,issues/,#, orPRbefore grabbing digits, so bare numbers,#N,PR N, and full URLs all resolve to1428.UX Journey
These are maintainer-facing automation workflows (no end-user UI). The relevant operator-facing flows:
Before
After
Architecture Diagram
Before
After
Connection inventory:
Task->Agentin 4 nodes; fan-out reachable again.pr-numberfilesnow routes torequest_changesclosefield gatesgh pr closeLabel Snapshot
risk: low(R2 alone isrisk: medium— governance behavior)size: Mworkflowsworkflows:maintainer-workflowsChange Metadata
bug(R4 isrefactor, R2 is behavior/security-adjacent governance)workflowsLinked Issue
.claude/archon/reviews/2026-07-16-maintainer-workflows-review.md, findings B1/B2/B3/B5/R1/R4/R2)Validation Evidence (required)
bun run validateexits 0; everyvalidate workflowswarning that motivated B1/B5 is gone; new empty-submission-contract test passes..archon/scripts/__tests__/tests are not part ofbun run validate(which is package-scoped) or CI — they run manually. Thedecidenode is inline-YAML with no unit harness (stated per instructions); thevalidate-schemascript contract it depends on is now test-pinned.Security Impact (required)
Compatibility / Migration
record-reviewstill writes the samereviewed-prs.jsonshape (incl. thegate_verdict: 'review'back-compat literal);.pr-numberartifact contract unchanged;decision.jsononly gains an additiveclosefield.Human Verification (required)
1428,#1428,PR 1428,the 2nd one, pr #1428, andgithub.com/coleam00/Archon/pull/1428— all resolve to1428(the naive grep returns00on the URL). Confirmed all four workflows validate clean. Ran the new schema-validator test (5 pass).files: []; a valid workflow yields a non-empty list; an unknown-provider workflow yieldsvalid: falsewith a non-empty list.ghwrite actions). Thegh pr closegating (R2) and marketplace auto-merge path were verified by reading, not by executing against a live PR.Side Effects / Blast Radius (required)
repo-triage,repo-triage-minimax,maintainer-review-pr,marketplace-pr-review-and-merge,maintainer-review-code-reviewcommand,marketplace-validate-schemascript.decideremains deterministic; auto-close now emits an explicit echo line naming whether it closed or flagged-for-maintainer.Rollback Plan (required)
afa19014) and can be reverted alone without affecting the other fixes.validate workflowswarnings reappearing (B1/B5 regression).Risks and Mitigations
decide; revert-in-place if you'd rather keep AI-only auto-close.Summary by CodeRabbit
Bug Fixes
Tests
Workflow Improvements