feat: implement issue #647 — [Phase 4] Promotion mechanism: informational to blocking after two clean audit cycles - #1084
feat: implement issue #647 — [Phase 4] Promotion mechanism: informational to blocking after two clean audit cycles#1084don-petry wants to merge 8 commits into
Conversation
…onal to blocking after two clean audit cycles
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
codeant-ai
Bot
added
the
size:XL
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # data cells, so NF is 8 (| c1 | c2 | c3 | c4 | c5 | c6 |). | ||
| if (NF < 8) next | ||
| cycle = trim($2) | ||
| if (cycle !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) next |
There was a problem hiding this comment.
Suggestion: The date check accepts impossible dates such as 2026-99-99, allowing invalid records to count toward the consecutive clean-cycle requirement. [data/type]
Assessment: 🟠 Major · 🔁 Occurrence: Rarely
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/agents-md-cycle-log.sh
**Line:** 71:71
**Comment:**
*Data Type: The date check accepts impossible dates such as `2026-99-99`, allowing invalid records to count toward the consecutive clean-cycle requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| printf "%s%s%s%s%s%s%s%s%s%s%s\n", \ | ||
| cycle, US, trim($3), US, trim($4), US, trim($5), US, trim($6), US, trim($7) |
There was a problem hiding this comment.
Suggestion: The details column is documented as free text, but any literal pipe is parsed as a new column and shifts the maintainer and clean flag fields. [type error]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/agents-md-cycle-log.sh
**Line:** 72:73
**Comment:**
*Type Error: The details column is documented as free text, but any literal pipe is parsed as a new column and shifts the maintainer and clean flag fields.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if [ -z "$maintainer" ] || [ "$maintainer" = "—" ] || [ "$maintainer" = "-" ]; then | ||
| printf 'cycle %s: no maintainer named in "Determined by" — every determination must name a maintainer\n' "$cycle" >&2 | ||
| rc=1; continue | ||
| fi |
There was a problem hiding this comment.
Suggestion: The attribution check accepts any non-empty text, so values that are not maintainer handles can satisfy the required named-maintainer audit condition. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Rarely
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/agents-md-cycle-log.sh
**Line:** 106:109
**Comment:**
*Api Mismatch: The attribution check accepts any non-empty text, so values that are not maintainer handles can satisfy the required named-maintainer audit condition.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if amcl_is_uint "$fps" && [ "$fps" -eq 0 ] && [ -n "$maintainer" ] \ | ||
| && [ "$maintainer" != "—" ] && [ "$maintainer" != "-" ]; then | ||
| clean=$((clean + 1)) | ||
| fi |
There was a problem hiding this comment.
Suggestion: eligibility counts rows as clean without checking the findings count or Clean? flag, so invalid rows can incorrectly establish promotion eligibility. [incorrect condition logic]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/agents-md-cycle-log.sh
**Line:** 141:144
**Comment:**
*Incorrect Condition Logic: `eligibility` counts rows as clean without checking the findings count or `Clean?` flag, so invalid rows can incorrectly establish promotion eligibility.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| n="$(grep -c . "$file" 2>/dev/null || true)" | ||
| printf '%s' "${n:-0}" | ||
| } |
There was a problem hiding this comment.
Suggestion: Any grep failure is converted to count zero, so an unreadable or failed structural findings file can be reported as a clean audit cycle. [error handling]
Assessment: 🟠 Major · 🔁 Occurrence: Rarely
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/compliance-audit.sh
**Line:** 3041:3043
**Comment:**
*Error Handling: Any `grep` failure is converted to count zero, so an unreadable or failed structural findings file can be reported as a clean audit cycle.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 4 of the AGENTS.md structural validation initiative, introducing an append-only cycle log, a helper script to validate and check promotion eligibility, and corresponding tests. The review feedback suggests robustly handling escaped pipe characters and Windows CRLF line endings in the log parser, as well as asserting exact non-zero exit statuses in BATS tests to avoid false positives.
| amcl_data_rows() { | ||
| local log="$1" | ||
| [ -f "$log" ] || return 0 | ||
| awk -F'|' -v US='\037' ' | ||
| function trim(s) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", s); return s } | ||
| { | ||
| # A markdown table row has an empty leading and trailing cell around 6 | ||
| # data cells, so NF is 8 (| c1 | c2 | c3 | c4 | c5 | c6 |). | ||
| if (NF < 8) next | ||
| cycle = trim($2) | ||
| if (cycle !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) next | ||
| printf "%s%s%s%s%s%s%s%s%s%s%s\n", \ | ||
| cycle, US, trim($3), US, trim($4), US, trim($5), US, trim($6), US, trim($7) | ||
| } | ||
| ' "$log" | ||
| } |
There was a problem hiding this comment.
If a maintainer includes an escaped pipe character (\|) in the Markdown table (e.g., in the false-positive details), awk -F'|' will still split the line on that character because it does not natively respect backslash escapes for field splitting. This shifts the subsequent fields (such as 'Determined by' and 'Clean?'), causing validation to fail with confusing errors.
We can prevent this by temporarily replacing escaped pipes with a non-printable character (like \001) before splitting, and then restoring them in the details field. Additionally, we should strip carriage returns (\r) from the input before passing it to awk to prevent Windows CRLF line endings from breaking the regex end-of-line anchor ($) in the cycle format validation.
| amcl_data_rows() { | |
| local log="$1" | |
| [ -f "$log" ] || return 0 | |
| awk -F'|' -v US='\037' ' | |
| function trim(s) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", s); return s } | |
| { | |
| # A markdown table row has an empty leading and trailing cell around 6 | |
| # data cells, so NF is 8 (| c1 | c2 | c3 | c4 | c5 | c6 |). | |
| if (NF < 8) next | |
| cycle = trim($2) | |
| if (cycle !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) next | |
| printf "%s%s%s%s%s%s%s%s%s%s%s\n", \ | |
| cycle, US, trim($3), US, trim($4), US, trim($5), US, trim($6), US, trim($7) | |
| } | |
| ' "$log" | |
| } | |
| amcl_data_rows() { | |
| local log="$1" | |
| [ -f "$log" ] || return 0 | |
| tr -d '\r' < "$log" | awk -F'|' -v US='\037' ' | |
| function trim(s) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", s); return s } | |
| { | |
| # Temporarily replace escaped pipes to prevent incorrect field splitting | |
| gsub(/\\\|/, "\001") | |
| # A markdown table row has an empty leading and trailing cell around 6 | |
| # data cells, so NF is 8 (| c1 | c2 | c3 | c4 | c5 | c6 |). | |
| if (NF < 8) next | |
| cycle = trim($2) | |
| if (cycle !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) next | |
| details = trim($5) | |
| gsub(/\001/, "\\|", details) | |
| printf "%s%s%s%s%s%s%s%s%s%s%s\n", \ | |
| cycle, US, trim($3), US, trim($4), US, details, US, trim($6), US, trim($7) | |
| } | |
| ' | |
| } |
References
- When parsing text with
awkin SHELL, strip carriage returns (\r) from the input (e.g., usingtr -d '\r') before passing it toawkto prevent Windows CRLF line endings from breaking regex end-of-line anchors ($).
| local log | ||
| log="$(write_log '| 2026-10-01 | 3 | 1 | anchor false alarm | | no |\n')" | ||
| run bash -c 'bash "$1" validate "$2" 2>&1' _ "$SCRIPT" "$log" | ||
| [ "$status" -ne 0 ] |
There was a problem hiding this comment.
Asserting a generic non-zero exit status (-ne 0) can lead to false positives if the command fails due to an unexpected error (such as a syntax error or command-not-found). It is safer and more robust to assert the exact expected non-zero exit status (which is 1 for validation failures).
[ "$status" -eq 1 ]
References
- In BATS tests, assert the exact expected non-zero exit status (e.g., [ "$status" -eq 1 ]) instead of a generic non-zero check (e.g., [ "$status" -ne 0 ]) to prevent script execution errors (like syntax errors or command-not-found) from causing false positives.
| local log | ||
| log="$(write_log '| 2026-10-01 | 0 | 0 | — | | yes |\n')" | ||
| run bash -c 'bash "$1" validate "$2" 2>&1' _ "$SCRIPT" "$log" | ||
| [ "$status" -ne 0 ] |
There was a problem hiding this comment.
Asserting a generic non-zero exit status (-ne 0) can lead to false positives if the command fails due to an unexpected error. Assert the exact expected non-zero exit status (1) instead.
[ "$status" -eq 1 ]
References
- In BATS tests, assert the exact expected non-zero exit status (e.g., [ "$status" -eq 1 ]) instead of a generic non-zero check (e.g., [ "$status" -ne 0 ]) to prevent script execution errors (like syntax errors or command-not-found) from causing false positives.
| local log | ||
| log="$(write_log '| 2026-10-01 | many | 0 | — | @alice | yes |\n')" | ||
| run bash "$SCRIPT" validate "$log" | ||
| [ "$status" -ne 0 ] |
There was a problem hiding this comment.
Asserting a generic non-zero exit status (-ne 0) can lead to false positives if the command fails due to an unexpected error. Assert the exact expected non-zero exit status (1) instead.
[ "$status" -eq 1 ]
References
- In BATS tests, assert the exact expected non-zero exit status (e.g., [ "$status" -eq 1 ]) instead of a generic non-zero check (e.g., [ "$status" -ne 0 ]) to prevent script execution errors (like syntax errors or command-not-found) from causing false positives.
| local log | ||
| log="$(write_log '| 2026-10-01 | 3 | 2 | two false alarms | @alice | yes |\n')" | ||
| run bash "$SCRIPT" validate "$log" | ||
| [ "$status" -ne 0 ] |
There was a problem hiding this comment.
Asserting a generic non-zero exit status (-ne 0) can lead to false positives if the command fails due to an unexpected error. Assert the exact expected non-zero exit status (1) instead.
[ "$status" -eq 1 ]
References
- In BATS tests, assert the exact expected non-zero exit status (e.g., [ "$status" -eq 1 ]) instead of a generic non-zero check (e.g., [ "$status" -ne 0 ]) to prevent script execution errors (like syntax errors or command-not-found) from causing false positives.
CodeAnt Nitpicks1 code suggestion1. The documented command omits the script path, so running it from the repository root fails with “command not found” unless
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: f94a5cf3a08ac2227a88f6bfece41cf06bbef63d
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds the informational->blocking promotion mechanism for AGENTS.md structural validation: a new pure, side-effect-free reader/validator (scripts/agents-md-cycle-log.sh), an append-only committed cycle log (ships empty/inert), a per-cycle finding count in the compliance-audit summary, and rules.json promotion metadata with toggle.enabled=false. The mechanism arms nothing (default_severity unchanged, check stays informational), touches no secrets/auth/crypto/DB, and passes all security scanners (CodeQL, SonarCloud, gitleaks, AgentShield). Downstream impact: (none). The two triage-flagged advisory findings are legitimate but non-blocking nits and do not warrant Tier-3 escalation.
Findings
- minor: amcl_data_rows uses
awk -F'|'which does not honor backslash-escaped pipes (|). If a maintainer later puts a literal pipe in the 'False-positive details' cell, NF increases and the maintainer/Clean? fields shift columns. Traced impact: amcl_validate_log fails CLOSED (rc=1 with a confusing error) rather than fabricating a clean cycle, and the shipped log is currently empty so there is no active path today. Recommend pre-substituting escaped pipes to a sentinel before splitting (then restoring) so future rows with pipes in free-text parse cleanly. Not a security bypass; safe to address in a follow-up. - minor: Four
validate-rejection tests assert[ "$status" -ne 0 ]instead of the exact[ "$status" -eq 1 ]. Since the CLI returns 1 for a malformed/unattributed row but 2 for usage/env errors (e.g. missing file),-ne 0could let an unintended rc=2 pass as a rejection false-positive. Tighten to-eq 1to assert the intended validation-failure path. Non-blocking maintainability nit. - info: MCP run_secret_scanning tool was not exposed in this environment; relied on the passing gitleaks CI check instead. No scan result fabricated.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: f94a5cf3a08ac2227a88f6bfece41cf06bbef63d
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds the informational->blocking promotion MECHANISM for AGENTS.md structural validation: a new pure/side-effect-free reader-validator (scripts/agents-md-cycle-log.sh), an append-only committed cycle log that ships EMPTY/inert, a per-cycle finding count in the compliance-audit summary, and rules.json promotion metadata with toggle.enabled=false. It arms nothing (default_severity unchanged, check stays informational), touches no secrets/auth/crypto/DB, and passes all scanners (CodeQL, SonarCloud, gitleaks, AgentShield) with all CI green. Downstream impact: (none). The two triage-flagged advisory findings are legitimate but non-blocking nits and do not warrant Tier-3 escalation.
Findings
- minor: amcl_data_rows uses
awk -F'|'which does not honor backslash-escaped pipes. If a future maintainer places a literal/escaped pipe in the free-text 'False-positive details' cell, NF increases and the maintainer/Clean? fields shift columns. Traced impact: amcl_validate_log fails CLOSED (rc=1 with a confusing error, e.g. clean-flag mismatch) rather than fabricating a clean cycle, and the shipped log is empty so there is no active path today. eligibility (report-only, never gates) could mis-count such a malformed row, but validate rejects the same log and promotion is human-gated with sign-off. Recommend pre-substituting escaped pipes to a sentinel before splitting (then restoring). Not a security bypass; safe to address in a follow-up. - minor: Four validate-rejection tests assert
[ "$status" -ne 0 ]instead of the exact[ "$status" -eq 1 ]. The CLI returns 1 for a malformed/unattributed row but 2 for usage/env errors; in these tests the log exists and args are well-formed so only rc=1 is reachable, but tightening to-eq 1asserts the intended validation-failure path and prevents an unintended rc=2 passing as a rejection. Non-blocking nit flagged by Gemini. - info: MCP run_secret_scanning tool was not exposed in this environment (only context7 MCP tools available); relied on the passing gitleaks CI check instead. No scan result fabricated.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at f94a5cf.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: f94a5cf3a08ac2227a88f6bfece41cf06bbef63d
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds a pure, bats-tested reader/validator for an append-only AGENTS.md cycle log plus docs, a disabled promotion toggle in the rule set, and a per-cycle finding count in the audit summary — the mechanism arms nothing. All 35 CI checks pass (ShellCheck, CodeQL, gitleaks, Agent Security Scan, SonarCloud, bats); GitHub secret-scanning MCP was not available so the gitleaks CI gate was relied upon (no result fabricated). Downstream impact is (none).
Findings
- MAJOR: awk -F'|' in amcl_data_rows does not honor backslash-escaped or literal pipes in the 'False-positive details' cell; a pipe there shifts the 'Determined by'/'Clean?' fields. This fails SAFE — the validator rejects the misaligned row loudly (maintainer/clean mismatch) rather than passing a fabricated clean cycle, and every row is authored in a maintainer-reviewed, git-tracked PR, so no eligibility spoofing is possible. Recommend hardening (temporarily substitute escaped pipes before splitting, restore after) to avoid confusing validation failures on legitimate entries.
- MINOR: validate-failure assertions use generic
[ "$status" -ne 0 ]; the malformed-row path always returns exactly 1, so tightening to-eq 1(per Gemini advisory) would prevent an unrelated env error (exit 2) from masquerading as the expected validation failure. Non-blocking.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at f94a5cf.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: f94a5cf3a08ac2227a88f6bfece41cf06bbef63d
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds an append-only cycle-log validator, docs, a disabled promotion toggle, and a per-cycle structural-finding count in the audit summary. The whole mechanism is inert on merge (toggle enabled=false, shipped log has zero data rows) and promotion stays human-gated and never automatic, so it fails safe. All CI is green (bats, ShellCheck, CodeQL, gitleaks, SonarCloud quality gate, Agent Security Scan) and no security anti-patterns are present; the triage/Gemini HIGH escaped-pipe finding is a real robustness bug but errs toward validation failure, cannot fabricate a clean cycle, and does not meet the HIGH security taxonomy — approving with findings recorded rather than escalating to Tier 3. Downstream impact: none.
Findings
- MAJOR: amcl_data_rows uses
awk -F'|'which does not honor Markdown's backslash-escaped pipe (|). If a maintainer puts a literal escaped pipe in the free-text 'False-positive details' cell, fields shift right so 'Determined by'/'Clean?' are misread. Traced direction: the shift makes the Clean? check read the maintainer handle, sovalidateFAILS (safe direction — it rejects the row and forces a fix); it cannot fabricate clean-cycles-met=true nor a false clean cycle, and nothing auto-promotes. Still worth fixing before real rows are appended: replace escaped pipes with a sentinel (e.g. �) before splitting on '|', then restore them in the details cell — the fix Gemini suggested. - MINOR: Four negative-path assertions use the generic
[ "$status" -ne 0 ]instead of the exact[ "$status" -eq 1 ]. validate returns 1 on a malformed/unattributed row, so an unexpected error (syntax error, command-not-found underset -euo pipefail) would still satisfy -ne 0 and mask a real failure. Tighten to -eq 1. Flagged 4x by Gemini. - INFO: run_secret_scanning MCP tool was not available in this environment (only context7 MCP exposed); no MCP secret scan performed. The gitleaks CI check is COMPLETED/SUCCESS and the diff introduces no credential-like content, so this is non-blocking.
- INFO: DOWNSTREAM_IMPACT is (none): no downstream consumer repos pin the shared surfaces this PR touches. Note that compliance-audit.sh and agents-md-rules.json are consumed internally; the changes to them are additive (new summary line, new JSON keys) and non-breaking.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at f94a5cf.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 316357f5e5ec8d9fea964bd47bb603f483fe87f9
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds an append-only AGENTS.md structural cycle-log reader/validator (scripts/agents-md-cycle-log.sh), a per-cycle structural-finding count in the compliance-audit summary, docs, and a promotion toggle in rules.json that ships DISABLED (toggle.enabled=false, default_severity unchanged) — the mechanism arms nothing and promotion stays human-gated. No secrets/auth/crypto/DB; all 35 CI checks are green (ShellCheck, CodeQL, gitleaks, AgentShield, SonarCloud quality gate, bats). The triage/Gemini HIGH escaped-pipe finding is a real robustness bug but fails CLOSED (validate rejects the misaligned row rather than fabricating a clean cycle) and is further defended by CI running validate on the shipped log, which is empty/inert — it does not meet the HIGH security taxonomy, so approving with findings recorded rather than escalating to Tier 3. Downstream impact: (none).
Findings
- MAJOR: amcl_data_rows uses
awk -F'|'which does not honor Markdown backslash-escaped or literal pipes in the free-text 'False-positive details' cell (col 4). A pipe there raises NF and shifts 'Determined by'/'Clean?' one column right. Traced impact: amcl_validate_log reads the shifted 'clean' as a maintainer handle, which matches neither 'yes' nor 'no', so validate FAILS (rc=1) — it fails safe, rejecting the row loudly rather than passing a fabricated clean cycle. amcl_clean_cycles_met (report-only, never gates) could mis-count such a row, but the same log fails validate, CI runs validate on the committed log, promotion requires explicit human sign-off, and the shipped log is empty — so there is no active bypass path. Recommend pre-substituting escaped pipes to a sentinel before splitting and restoring them after, per the Gemini advisory. Safe to fix in a follow-up. - MINOR: Four validate-rejection tests assert generic
[ "$status" -ne 0 ]instead of the exact[ "$status" -eq 1 ]. The CLI returns 1 for a malformed/unattributed row but 2 for usage/environment errors; in these tests the log exists and args are well-formed so only rc=1 is reachable, but tightening to-eq 1asserts the intended validation-failure path and prevents an unrelated rc=2 (e.g. a script error under set -euo pipefail) from masquerading as the expected rejection. Non-blocking maintainability nit flagged 4x by Gemini. - INFO: GitHub Secret Protection MCP tool (run_secret_scanning) was not exposed in this environment (only the context7 MCP server is available), so no MCP secret scan was performed. The gitleaks CI check is COMPLETED/SUCCESS and the diff introduces no credential-like content (shell/markdown/JSON about an audit cycle log). No scan result fabricated.
- INFO: DOWNSTREAM_IMPACT is (none): no external consumer repos pin the shared surfaces this PR touches. compliance-audit.sh and agents-md-rules.json are consumed internally; the edits are additive (new summary line + pure counter function; new JSON keys) and non-breaking.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 316357f.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) promotion-mechanism PR: adds an append-only, git-committed cycle log plus a pure, bats-tested reader/validator (scripts/agents-md-cycle-log.sh) that reports the two-clean-cycle precondition but never promotes, never blocks, and never writes. The toggle ships disabled and the check stays informational, so the blast radius is confined to a maintainer-authored, PR-reviewed doc. All CI is green (ShellCheck, gitleaks, CodeQL, SonarCloud, bats, AgentShield), test coverage is thorough, and downstream impact is (none). Triage escalated over an unaddressed HIGH Gemini advisory about awk -F'|' not respecting escaped pipes; I adjudicate it as a genuine but low-impact robustness gap: because prose lands in the details column only when fps>0 (which already breaks the clean streak), field-shifting cannot fabricate a clean-cycle claim — it only yields a confusing validation error the PR author sees immediately, in a reviewed PR. Not a security-taxonomy HIGH; MEDIUM logic change with all gates passing, so approving with the advisory recorded as a non-blocking follow-up.
Findings
- minor: awk -F'|' does not honor backslash-escaped pipes (|) in the free-text 'False-positive details' column. A literal pipe there shifts fields 5-7 (details/maintainer/clean), producing a confusing validation failure. Recommended follow-up: temporarily substitute | with a sentinel (e.g. \001) before -F'|' splitting and restore it in the details cell, per the Gemini advisory. Impact is bounded: the tool is informational-only, the input is maintainer-authored in a reviewed PR, and — since details prose only appears when fps>0, which already fails the clean-cycle check — the bug cannot falsely inflate clean-cycle eligibility. Does not undermine the tamper-evidence guarantee.
- info: Gemini MEDIUM advisories on tests/agents_md_cycle_log.bats:72,82,90,99 suggest asserting the exact failure code ([ "$status" -eq 1 ]) instead of the generic [ "$status" -ne 0 ], to avoid masking syntax/command-not-found errors as expected failures. Reasonable tightening; non-blocking.
- info: run_secret_scanning MCP tool is not exposed in this environment (only context7 MCP available); the MCP validated-secret scan was skipped. The gitleaks CI check passed (SUCCESS) and manual diff review found no credentials, tokens, or .env content.
- info: mergeStateStatus is BLOCKED while mergeable is MERGEABLE — expected pending-required-review state for this review flow, not a gate failure. All statusCheckRollup entries are SUCCESS or SKIPPED.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
Superseded by automated re-review at
|
Superseded by automated re-review at 2877da2.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) promotion mechanism: a pure, bats-tested reader/validator for an append-only AGENTS.md structural cycle log, plus a per-cycle finding count in the audit summary and doc/rule-set updates. Ships INERT (promotion.toggle.enabled=false) — informational-only, arms nothing, never auto-promotes. All CI green (ShellCheck, bats, CodeQL, gitleaks, SonarCloud); no auth/secrets/crypto/migrations. No downstream consumers impacted (DOWNSTREAM_IMPACT: none). run_secret_scanning MCP tool not exposed in this env, so that scan was skipped (no credential material in the diff anyway). The escalated awk edge case is real but fails closed and is low-impact, so no security-audit escalation is warranted.
Findings
- minor: awk -F'|' in amcl_data_rows does not respect markdown-escaped pipes (|). An escaped pipe in the free-text 'False-positive details' cell over-splits the row and shifts the 'Determined by'/'Clean?' fields. Impact is bounded: it fails CLOSED (the shifted Clean? value won't match yes/no, so amcl_validate_log rejects the row rather than fabricating a clean cycle), affects only a hand-authored/reviewable log, and the mechanism ships inert. Recommend pre-substituting | to a sentinel (e.g. 0x01) before splitting and restoring it in the details cell, so a legitimate escaped-pipe detail validates instead of erroring confusingly.
- minor: Validation-rejection tests assert [ "$status" -ne 0 ] rather than the documented exit code (validate returns 1 on a malformed/unattributed row, 2 on usage/env errors). -ne 0 would let a return-2 env error masquerade as a validation failure. Prefer [ "$status" -eq 1 ] for the malformed-row cases. Advisory only.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Review mode: triage-approved (single reviewer)
Summary
Confirmation review (triage-approved mode) of the Phase 4 promotion mechanism for #647. Adds an append-only, committed cycle log (docs/initiatives/agents-md-validation-cycle-log.md), a pure sourceable reader/validator (scripts/agents-md-cycle-log.sh: validate + eligibility, never promotes), a per-cycle structural-finding count in the compliance-audit summary, a disabled promotion.toggle in the rule set, and bats coverage for all of it. Everything ships inert: toggle.enabled=false, no branch-protection change, tests assert the committed log is not promotion-eligible on merge. Triage assessment confirmed; all CI green; prior full cascade approved this same SHA.
Linked issue analysis
Issue #647 is substantively addressed: AC1 — structural_finding_count() records the machine-counted per-cycle finding count in the audit summary; AC2 — agents-md-validation.md §4.1–4.3 document the two-clean-cycles + maintainer sign-off gate and the promotion checklist; AC3 — the discrete toggle (promotion.toggle in agents-md-rules.json, linter --mode failing, required-status-check wiring) is delivered documented-but-disabled; AC4 — nothing is armed and eligibility output explicitly states auto_promote=false; AC5 — the cycle log is append-only, committed, and the validator rejects any determination (including clean cycles) without a named maintainer.
Findings
No blocking findings. Secret-scan MCP tool unavailable in this environment; the gitleaks CI check passed. Ten unresolved bot suggestion threads (codeant-ai, gemini-code-assist) remain: edge-case hardening nitpicks — escaped pipes in the details column, impossible-date regex acceptance, generic non-zero exit assertions in tests, grep-failure-to-zero in the counter. All are minor: every cycle-log row lands via a maintainer-reviewed PR, so the human gate covers these parser edge cases, and eligibility checks the false-positive count directly rather than trusting the derived Clean? flag. The dev-lead agent evaluated these threads across three cycles and concluded no changes were needed; the prior full cascade approved this exact SHA with them open. Non-blocking.
CI status
All required checks green at 2877da2: ShellCheck, bats (both suites incl. the new agents_md_cycle_log tests), Lint, CodeQL, SonarCloud quality gate (0 new issues), Secret scan (gitleaks), Agent Security Scan, AgentShield, duplicate-decl-gate. Remaining entries are expected skips (ecosystem-specific dependency audits, dependabot-automerge).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 promotion mechanism for the AGENTS.md structural check: a new pure/tested cycle-log reader-validator, an append-only committed log, per-cycle finding count in the audit summary, and a promotion toggle that ships DISABLED (informational-only; no branch-protection change). All CI green (ShellCheck, bats, CodeQL, gitleaks, SonarCloud, Agent Security Scan) and SonarCloud Quality Gate passed. No downstream consumers impacted (DOWNSTREAM_IMPACT=(none)). The GitHub run_secret_scanning MCP tool is not available in this environment; gitleaks CI already passed, so no MCP scan was performed.
Findings
- minor: awk -F'|' in amcl_data_rows does not honor backslash-escaped pipes (|) in the free-text 'False-positive details' cell, so such a row shifts columns 5-6. Note: this fails CLOSED — validate rejects the row (the 'Clean?' field reads a maintainer handle that matches neither yes/no) rather than accepting a bad one. Finding-count and false-positive-count columns precede details and are unaffected, and clean cycles carry empty details, so it cannot fabricate a clean-cycle claim. Worth hardening (e.g. swap escaped pipes to \001 before splitting, restore after) but not a security or blocking issue.
- minor: Four validate-failure tests assert generic non-zero exit ([ "$status" -ne 0 ]) rather than the exact code 1. validate returns 1 for a malformed/unattributed row but 2 for usage/env errors, so an unexpected env failure could pass these assertions. Tightening to -eq 1 is more precise. Non-blocking.
- info: Mechanism ships inert by design: promotion.toggle.enabled=false, audit remains informational, no branch-protection change, and eligibility always exits 0 (report, not gate). Tamper-evidence property (append-only committed log + mandatory maintainer attribution + clean-flag/count consistency) is enforced by validate and covered by tests. Human-gated promotion is preserved; no automatic flip.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) delivers the append-only cycle-log validator, per-cycle finding count, and a discrete blocking toggle that is delivered disabled — the structural check remains informational and arms nothing on merge. CI is fully green (ShellCheck, bats, CodeQL, gitleaks, SonarCloud all pass) and the change is well-structured and thoroughly tested. Risk is MEDIUM with no security dimension, so no Tier 3 audit is warranted.
Findings
- MAJOR: awk -F'|' field splitting does not respect Markdown-escaped pipes (|) in a table cell (e.g. the free-text 'False-positive details' column). An escaped or literal pipe in details shifts subsequent fields (maintainer, clean) and makes validation fail with a confusing error. Verified fail-safe: 'fps' is read from column $4 (before 'details' at $5), so the false-positive count is always parsed correctly and the shift can only REJECT a row (mismatched Clean?), never fabricate a clean-cycle pass. Impact is limited because the mechanism ships inert (no rows, promotion armed nowhere) and is informational-only. Recommend the bot's fix: swap escaped pipes to a sentinel (e.g. \001) before -F'|' splitting and restore them in the details cell. Non-blocking.
- MINOR: Gemini advisory (medium): several bats assertions use a generic non-zero check ([ "$status" -ne 0 ]) for expected validation failures. Prefer asserting the exact code ([ "$status" -eq 1 ]) so a syntax/command-not-found error cannot masquerade as the expected failure. amcl_main returns 1 for malformed rows and 2 for usage/env errors, so the exact code is well-defined. Non-blocking.
- INFO: run_secret_scanning MCP tool is not exposed in this environment (only the context7 MCP server is available), so the optional MCP secret scan was skipped. The gitleaks CI check passed and the diff contains only shell parsing logic, docs, JSON config, and tests — no credential material.
- INFO: DOWNSTREAM_IMPACT is (none): no downstream consumer repos pin the shared surfaces changed here. compliance-audit.sh and the rule set are org-internal to this repo.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds an append-only cycle-log reader/validator plus docs and per-cycle finding count in the audit summary; the mechanism ships inert (informational, human sign-off + manual toggle required — arms nothing). All CI is green (gitleaks, CodeQL, ShellCheck, SonarCloud, CodeRabbit). Downstream impact is (none); run_secret_scanning MCP tool was not available so the gitleaks CI check stands alone. The triage's HIGH advisory (awk -F'|' escaped-pipe parsing) is on analysis a fail-closed robustness nit — a shifted field breaks the Clean? check and validate returns 1, so it cannot forge an unattributed or fake-clean row — not a security bypass; the four test-assertion nits are minor. No auth/secrets/crypto, migrations, injection, or Actions security smells. Approving.
Findings
- MINOR: (
scripts/agents-md-cycle-log.sh:76) amcl_data_rows usesawk -F'|'which does not honor Markdown escaped pipes (|). A | in the 'False-positive details' cell shifts subsequent fields, so the maintainer/Clean? cells misalign. Impact is fail-closed: the Clean? check sees a non-yes/no value andvalidatereturns 1, surfacing a confusing (not silent) failure on the appending PR — it cannot be used to smuggle an unattributed or fake-clean row past validation. Worth hardening (e.g. temporarily substitute | with \001 before splitting, restore in the details cell) but non-blocking given the fail-closed behavior and the informational, human-gated design. - MINOR: (
tests/agents_md_cycle_log.bats:72) Four validation-failure assertions use[ "$status" -ne 0 ]instead of the exact[ "$status" -eq 1 ]. amcl_main returns 1 for a malformed/unattributed row but 2 for usage/environment errors, so a generic non-zero check could pass on the wrong failure mode. Tightening to -eq 1 makes the tests assert the intended validation path. - INFO: (
scripts/agents-md-cycle-log.sh) Mechanism is delivered disabled: promotion.toggle.enabled=false, default_severity unchanged, linter stays informational, no branch-protection change. eligibility() always exits 0 and restates maintainer_sign_off_required=true / auto_promote=false. Confirmed no auto-promotion path is armed by this PR. AMCL_FS uses 0x1f (non-whitespace) so empty cells survive the read, preserving the attribution guard. - INFO: All status checks pass (SUCCESS/SKIPPED), including Secret scan (gitleaks), CodeQL, ShellCheck, SonarCloud, CodeRabbit, Agent Security Scan. SonarCloud Quality Gate passed with 0 new issues. mergeStateStatus BLOCKED reflects the pending review gate. run_secret_scanning MCP tool not available in this environment; not fabricating a result.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2877da21d7bf852ce16bdac39e6911a1af5ceaa6
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Findings
- No specific findings
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 2877da2.
Automated activity budget exhausted — human attention neededThis PR has reached 10 automated actions (agent commits + review cycles + acks) since the last human interaction, without converging. To prevent a runaway loop (see #926 / the #860 post-mortem), all automated commits, reviews, and acknowledgements on this PR are now paused, auto-merge is disabled, and Re-engaging is human-gated. A human reviewing, commenting, or pushing to this PR resets the budget; a machine action will not. Removing |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 6af3b1111ba6526b9f47d31a986cc4bf853a153f
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Phase 4 (#647) adds a pure, bats-tested reader/validator for an append-only AGENTS.md structural cycle log plus a per-cycle finding count in the compliance-audit summary; the promotion toggle ships disabled (toggle.enabled=false) and no branch-protection/linter mode is armed. All 28 CI checks pass (gitleaks, CodeQL, SonarCloud, ShellCheck, bats) and the feature is informational-only. The Gemini HIGH advisory about escaped pipes in awk -F'|' parsing is a genuine robustness defect but fails safe: an escaped pipe can appear only in the false-positive details cell of a non-clean (fps>0) row, and the resulting field shift makes validate reject the row loudly rather than fabricate a clean/passing cycle — it cannot bypass the gate. Downstream impact: none. MCP run_secret_scanning tool was not available in this environment; relied on the green gitleaks + CodeQL checks.
Findings
- MAJOR: awk -F'|' in amcl_data_rows does not respect markdown-escaped pipes (|). A literal pipe in the free-text 'False-positive details' cell shifts the Determined-by/Clean? fields rightward. Impact is fail-safe (validate rejects the row with a confusing error rather than faking a clean cycle, and the false-positive count in $4 precedes details so eligibility is unaffected), but a maintainer legitimately quoting a pipe in details will hit a confusing validation failure. Recommend substituting escaped pipes to a sentinel (e.g. \001) before splitting and restoring them in the details cell, as the advisory suggests. Non-blocking: the log is currently empty and the mechanism arms nothing. (scripts/agents-md-cycle-log.sh:76)
- MINOR: Four validate-failure tests assert a generic non-zero status ([ "$status" -ne 0 ]) instead of the exact code. amcl_main returns 1 for a malformed/unattributed row (2 is reserved for usage/env errors), so asserting [ "$status" -eq 1 ] would prevent a usage/setup error (exit 2) from masquerading as a validation failure. Flagged by Gemini at tests/agents_md_cycle_log.bats lines ~669,679,687,696. Non-blocking hygiene improvement. (tests/agents_md_cycle_log.bats:669)
- INFO: No secrets, auth/credential/crypto, or DB changes. GitHub Actions change only extends trigger paths and adds a bats file to the existing test runner (no new permissions, no pull_request_target, no untrusted checkout). awk parsing operates on a committed, PR-reviewed markdown file (no untrusted-input injection surface). Promotion toggle ships disabled; no branch-protection change. MCP run_secret_scanning unavailable; gitleaks and CodeQL checks are green. (n/a)
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
Superseded by automated re-review at 6af3b11.
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
2 participants
User description
Closes #647
Implemented by dev-lead agent. Please review.
CodeAnt-AI Description
Add an auditable, human-gated path for evaluating AGENTS.md checks for blocking enforcement
What Changed
Impact
✅ Auditable clean-cycle decisions✅ No automatic enforcement changes✅ Clear maintainer attribution💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.