Skip to content

fix(#140): sync REVIEW_SPEC_HIERARCHY with AGENTS.md - #183

Merged
JohnStrunk merged 2 commits into
mainfrom
agent/140-spec-hierarchy-sync
Sep 23, 2026
Merged

JohnStrunk merged 2 commits into
mainfrom
agent/140-spec-hierarchy-sync

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

REVIEW_SPEC_HIERARCHY in .fullsend/harness/review.yaml listed 8 of the 13 paths in AGENTS.md's Specification document hierarchy. The gap dates from the env var's introduction in PR #124 and grew when PR #151 added docs/architecture/drafting-table-wms.md without updating the review harness.

This change:

  • Updates REVIEW_SPEC_HIERARCHY to the full 13-path AGENTS.md list (git-integration.md, validation-rules.md, drafting-table-wms.md, drafting-table-ux.md, and agent-harness/ were missing).
  • Adds a comment that the two lists must stay in 1:1 sync.
  • Adds a spec-hierarchy-sync local pre-commit hook (scripts/check_spec_hierarchy.py) and wires it through scripts/lint.py so a future hierarchy addition that is not also added to review.yaml fails in the same change.

.fullsend/ is in REVIEW_PROTECTED_PATHS; closer-than-usual review of this file is expected.

Testing

  • python3 scripts/check_spec_hierarchy.py against the real files (pass).
  • bash tests/lint/test_violations.sh (23 passed), including match, order-independence, missing/extra paths, missing heading/key, and lint.py hook wiring.
  • python3 scripts/lint.py --files on the changed files (all applicable hooks passed, including the new hook).
  • python3 scripts/lint.py --check-parity (pass).

pre-commit run could not fetch remote hook repositories (HTTP 403); equivalent hooks were run via scripts/lint.py.


Closes #140

Post-script verification

  • Branch is not main/master (agent/140-spec-hierarchy-sync)
  • Secret scan passed (gitleaks — bd40e6ae69886064be2950e78d35b21846cdab74..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

REVIEW_SPEC_HIERARCHY in .fullsend/harness/review.yaml listed 8 of
the 13 specification-hierarchy paths in AGENTS.md. The missing
entries (git-integration.md, validation-rules.md,
drafting-table-wms.md, drafting-table-ux.md, and agent-harness/)
have been incomplete since the env var was introduced, and a later
hierarchy addition drifted again without detection.

Update the env var to the full AGENTS.md list, add a comment that
the two must stay in sync, and add a spec-hierarchy-sync pre-commit
hook (wired through scripts/lint.py) that asserts set equality so
the next hierarchy change cannot silently drop out of review.

Note: pre-commit could not fetch remote hook repositories
(HTTP 403). Equivalent hooks were run via python
scripts/lint.py --files on the changed files and passed.

Closes #140
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team September 23, 2026 19:23
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Sep 23, 2026
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: redhat-et/ProtoBot/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: edec5db4-9854-44ac-8e9d-f21c18cdaa12

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Ended 7:25 PM UTC

Commit: ec6e3a1 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 23, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Score unchanged from the prior assessment (2/moderate): Tier 1 metadata is identical (5 files, 4 protected config/tooling paths, no security-sensitive or dependency changes, bot author), Tier 2 git history shows low churn/fix-revert/coupling for the touched files, and Tier 3 shows the PR tightly matches its linked issue with full acceptance-criteria coverage; the single intervening follow-up commit (cosmetic parentheses removal) introduces no new protected paths or dependencies and does not move the composite off 2.

Previous run

Risk Assessment: moderate (2/5)

Details

Bot-authored PR touching several protected config paths but with no security-sensitive files, no CI/dependency changes, reasonable test coverage, a stable/low-conflict git history for the touched files, and a PR that closely matches the scope and acceptance criteria of its linked, unlabeled issue, yielding a moderate composite risk.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review

Findings

Medium

No other findings. The single follow-up commit since the prior review correctly addressed its low-severity code-style finding (the redundant parentheses around the default_files value for spec-hierarchy-sync in scripts/lint.py were removed, now matching the bare raw-string style of sibling _REGISTRY entries). Independent correctness and style-conventions passes over the full diff found no new issues, and the new REVIEW_SPEC_HIERARCHY value was cross-checked against the current AGENTS.md "Specification document hierarchy" list (13 entries) — it matches exactly.

Previous run

Review

Findings

Medium

Low

  • [code-style] scripts/lint.py:507 — The new spec-hierarchy-sync registry entry's default_files value is wrapped in redundant parentheses ((r"^(AGENTS\.md|\.fullsend/harness/review\.yaml)$")), while every other default_files definition in _REGISTRY (lines 418, 481, 495) uses a bare raw string literal without grouping parentheses. This is cosmetic only — the parenthesized expression still evaluates to a plain str (there is no trailing comma inside the parens, so it is not a tuple), so re.compile() receives a string either way.
    Remediation: Remove the redundant parentheses to match sibling hook definitions: "default_files": r"^(AGENTS\.md|\.fullsend/harness/review\.yaml)$",.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:27 PM UTC · Completed 7:40 PM UTC

Commit: ec6e3a1 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.29

Remove redundant parentheses around the default_files raw string
literal for spec-hierarchy-sync in scripts/lint.py to match sibling
hook entries in _REGISTRY.

Note on [protected-path] finding: changes to .fullsend/harness/review.yaml
and related files are intentional and central to issue #140, awaiting
human approval as required by project governance.

Note: pre-commit could not fetch remote hook repositories (HTTP 403).
Equivalent hooks were run via python scripts/lint.py --files scripts/lint.py
and passed.

Addresses #183
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed both review findings: removed redundant parentheses around default_files for spec-hierarchy-sync in scripts/lint.py to match sibling hook definitions; retained intentional protected-path changes in .fullsend/harness/review.yaml awaiting human approval as required by project governance.

Fixed (1):

  1. [code-style] scripts/lint.py:507 — redundant parentheses in default_files (scripts/lint.py): Removed redundant outer grouping parentheses from the default_files raw string literal in the spec-hierarchy-sync registry entry to match sibling hook definitions.

Disagreed (1):

  1. [protected-path] .fullsend/harness/review.yaml — governed protected paths require human approval: Modifying .fullsend/harness/review.yaml and related files is the intentional core purpose of PR REVIEW_SPEC_HIERARCHY in .fullsend/harness/review.yaml is stale on arrival — missing 4 of AGENTS.md's 12 spec-hierarchy entries, including the directory PR #124 itself added #140 (syncing REVIEW_SPEC_HIERARCHY with AGENTS.md). The changes are kept as-is and await human approval as required by project governance.

Tests: passed

Decision points

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 7:40 PM UTC · Completed 7:50 PM UTC

Commit: ec6e3a1 · View workflow run →

Runtime: pi · Model: google-vertex/gemini-3.8-flash → gemini-3.8-flash · Effort: high · Cost: $0.34

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:50 PM UTC · Completed 7:59 PM UTC

Commit: ff60e6d · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.49

@JohnStrunk
JohnStrunk added this pull request to the merge queue Sep 23, 2026
Merged via the queue into main with commit f7c5814 Sep 23, 2026
35 checks passed
@JohnStrunk
JohnStrunk deleted the agent/140-spec-hierarchy-sync branch September 23, 2026 20:21
@fullsend-ai-retro

Copy link
Copy Markdown

PR #183 closed issue #140 (REVIEW_SPEC_HIERARCHY drift) cleanly and the review/fix loop performed well. Timeline: a prior retro (on PR #124) filed #140; a /fs-triage re-verification on 2026-09-23 confirmed the drift had grown (PR #151 added a doc without updating review.yaml) and recommended an automated sync check, not just a one-off fix. The code agent's implementation in PR #183 did exactly that: it updated REVIEW_SPEC_HIERARCHY to the full 13-path list and added a spec-hierarchy-sync pre-commit hook (wired through scripts/lint.py, with 23 new test cases) so the gap can't silently recur — a good example of an agent generalizing a fix into prevention. The review agent correctly flagged the .fullsend/harness/review.yaml change as a protected-path finding requiring human approval (expected, by design) and caught one real, if trivial, code-style nit (redundant parentheses in a new registry entry, inconsistent with sibling entries). The fix agent fixed the style nit and correctly pushed back on the protected-path finding, explaining it was intentional and awaiting human sign-off rather than something to revert — exactly the right behavior. Human reviewer JohnStrunk approved without further comment after this exchange, and the PR merged in under an hour from open to merge. No issues found with this portion of the workflow.

One real inefficiency showed up upstream of PR #183, not in it: the first /fs-code dispatch on issue #140 (run 35893949167, $3.39, xai/grok-4.6) was hit by 65 repeated 429 rate-limit errors from the model provider starting from its first model call. The harness auto-restarted the agent process once, but the second attempt also drowned in 429s and the run ended in an internal error state before writing any diff. Because git diff on the agent's branch was empty, the post-script reported this to the human as ‘No PR created — agent determined no changes needed,’ which reads as a reasoned decision rather than a crashed run. The human (reasonably, given that framing) didn't retry /fs-code for about 2 hours, adding avoidable latency before the real fix (PR #183) was produced by a second, successful /fs-code run.

This is not a new finding: it's strong corroborating evidence for three already-open upstream issues in fullsend-ai/fullsend — #2379 ('Auto-retry code agent when terminated by transient LLM API errors (429)'), #6963 ('Surface code agent reasoning when a run produces no file changes'), and #7556 ('Release cadence stalled 13+ days after v0.43.0, letting already-merged 429-retry and false-success fixes recur in production' — this repo is pinned to v0.43.0 per .github/workflows/fullsend.yaml, consistent with #7556's claim that fixes for exactly this class of bug are merged upstream but not yet released). #6877 and #769 in the same repo track closely related misleading-status-message variants. No new proposal filed for this; recommend the retro/platform team prioritize #7556 given it appears to be blocking the actual fix from reaching repos like this one.

No other gaps found. Searched for prior issues on protected-path friction (#180, open, but not implicated here — this finding was Medium severity and the human-approval gate worked as intended), cosmetic-finding fix-cycle cost (#149, open, about effort tiering — not clearly implicated since the wasted cost here was provider-side, not model-tier-related), and found none for the two near-simultaneous pull_request_target dispatch runs on PR open (one cancelled almost immediately) — this looks like benign concurrency-group deduplication of a duplicate webhook delivery, not a costly or novel bug, so no proposal was filed for it.

@fullsend-ai-retro

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:22 PM UTC · Completed 8:29 PM UTC

Commit: ff60e6d · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.19

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

Labels

ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment risk/moderate PR risk: moderate

Projects

None yet

1 participant