docs!: add ADR 0088 for CEL-guarded overlays in the harness schema - #6237
Conversation
PR Summary by QodoAdd ADR 0088 for tracker/forge harness config split
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 5:49 PM UTC · Completed 6:04 PM UTC Commit: |
Site previewPreview: https://66ff47b4-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsHigh
Low
Next steps:
Previous run (5)ReviewFindingsLow
Previous run (6)ReviewFindingsMedium
Low
Labels: PR adds an ADR about the harness config schema (tracker/forge split) |
|
Replying in reference to #6237 (comment) Addressed the architecture.md, multiple-decisions, and Consequences-bullets findings on their own threads:
On the "exceeds 100 lines" point — same reasoning as the multiple-decisions thread: I'm keeping this as one ADR since the rename, |
|
Replying in reference to #6237 (comment) Addressed the
On the label point — |
|
🤖 Finished Review · ✅ Success · Started 6:22 PM UTC · Completed 6:38 PM UTC Commit: |
eb94141 to
51c0c2c
Compare
|
🤖 Review · Commit: |
51c0c2c to
3249e7d
Compare
|
🤖 Finished Review · ✅ Success · Started 2:45 AM UTC · Completed 3:03 AM UTC Commit: |
Superseded by updated review
- Switch from merge-all to first-match-wins overlay semantics - Add CEL environment section with runtime.forge, event, and config variables - Reference normevent.Event typed struct explicitly - Add config.forge to runtime.forge resolution precedence (--forge flag > config.forge > CI env vars) - Document combined-entry pattern for cross-concern scenarios Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · Commit: |
Adds a tracker: harness key sibling to forge:, renaming ForgeConfig to PlatformConfig and generalizing its merge pipelines, so a code agent can read from one tracker (e.g. JIRA) while writing to an unrelated forge. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Fix Load()/LoadWithOpts function reference, add OpenShell.Profiles to the precedence-rules field list, and link ADR 0088 from architecture.md's Agent Harness "Decided:" list. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Replace the tracker/forge split proposal with the overlays approach from @ifireball's review. Instead of adding a second platform-keyed block (tracker:), add a general overlays: list where each entry has a CEL when: expression and the same override fields as ForgeConfig. This is more flexible — harnesses can condition on any event property, not just the forge platform — and deprecates forge: rather than expanding it. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Fix broken link in architecture.md pointing to old filename - Correct forge-to-overlay equivalence: use forge.platform instead of event.source.system and document the divergence for cross-system events - Clarify base composition mutual exclusion applies post-merge and document the migration path for base harnesses using forge: - Specify deduplication semantics for overlay list merges Addresses review feedback on #6237
- Switch from merge-all to first-match-wins overlay semantics - Add CEL environment section with runtime.forge, event, and config variables - Reference normevent.Event typed struct explicitly - Add config.forge to runtime.forge resolution precedence (--forge flag > config.forge > CI env vars) - Document combined-entry pattern for cross-concern scenarios Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
1c6c716 to
f16b496
Compare
|
🤖 Finished Review · ✅ Success · Started 8:41 PM UTC · Completed 9:02 PM UTC Commit: |
maruiz93
left a comment
There was a problem hiding this comment.
I think this is a nice change. Posted some low severity comments.
|
🤖 Finished Retro · ✅ Success · Started 2:06 PM UTC · Completed 2:23 PM UTC Commit: |
Retro: PR #6237 -- ADR 0088 for CEL-guarded overlaysTimelinePR #6237 added ADR 0088 proposing CEL-guarded overlays in the harness schema. Over 4 days (Aug 14--18), the PR underwent a major architectural pivot: the original tracker/forge split design was approved by the review agent (Runs 1--2) but then rejected by human reviewer @ifireball on Aug 16, who proposed generalizing to CEL-guarded conditional sections. @ralphbean rewrote the ADR entirely on Aug 17. The review agent ran 8 times total, the fix agent was dispatched 3 times, and ~45 workflow dispatches were skipped immediately. The review agent produced 22 findings across its runs: 7 were substantively fixed (4 by the fix agent, 3 by the author), 6 were stale duplicates of already-addressed findings (27%), 5 were deferred as follow-ups for the implementation PR, 3 were acknowledged as intentional, and 1 valid medium-severity finding (algorithm-logic-consistency contradiction in the base composition ordering section) was merged without acknowledgment. The third fix dispatch (run 32069045748) failed silently: the pre-fetch step found a 1-byte review body and exited with "Bot-triggered run but review body is empty -- nothing to fix." No comment was posted, so the author had no signal that the fix agent did not run. Combined with the review agent's outstanding CHANGES_REQUESTED, this left the valid medium-severity finding unaddressed through merge. Evidence for existing issuesAll improvement opportunities are already tracked. Specific evidence from this PR:
Autonomy readinessThe review agent reliably detected broken links, internal prose inconsistencies, and API reference errors -- high-value mechanical checks for docs PRs. However, it approved an architecturally flawed design (Runs 1--2) that a human reviewer correctly rejected, and it could not surface implementation feasibility concerns that human reviewers identified. For ADR PRs proposing new architecture, human review remains essential and the review agent's approval should not be treated as a quality signal for design soundness. No new proposalsAll identified improvements are tracked by existing issues. No novel proposals are warranted. |
Temporary addition to unblock the triage-agent PoC for JIRA issues. The code agent remains blocked on a longer-term forge abstraction design currently under debate in fullsend-ai#6237. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Add missing JIRA_DUPLICATE_TRANSITION, JIRA_NOT_PLANNED_TRANSITION, JIRA_SPLIT_TRANSITION to forge.jira.env.runner in harness/triage.yaml so the runner passes transition vars to the post-script - Revert docs/triage.md heading and terminology from "Multi-tracker" back to "Multi-forge" — the tracker/forge harness-split was rejected upstream (fullsend-ai/fullsend#6237) and the harness still uses forge: as the top-level key - Fix LOCAL.md Jira walkthrough to use FULLSEND_FORGE instead of FULLSEND_TRACKER - Add description fields to triage-result schema documenting the dual format for duplicate_of (integer vs Jira key) and repo (org/repo path vs bare Jira project key) - Remove .gitignore scope-creep additions (.claude, openspec) unrelated to Jira tracker support Addresses review feedback on #827
- Add missing JIRA_DUPLICATE_TRANSITION, JIRA_NOT_PLANNED_TRANSITION, JIRA_SPLIT_TRANSITION to forge.jira.env.runner in harness/triage.yaml so the runner passes transition vars to the post-script - Revert docs/triage.md heading and terminology from "Multi-tracker" back to "Multi-forge" — the tracker/forge harness-split was rejected upstream (fullsend-ai/fullsend#6237) and the harness still uses forge: as the top-level key - Fix LOCAL.md Jira walkthrough to use FULLSEND_FORGE instead of FULLSEND_TRACKER - Add description fields to triage-result schema documenting the dual format for duplicate_of (integer vs Jira key) and repo (org/repo path vs bare Jira project key) - Remove .gitignore scope-creep additions (.claude, openspec) unrelated to Jira tracker support Addresses review feedback on #827
- Add missing JIRA_DUPLICATE_TRANSITION, JIRA_NOT_PLANNED_TRANSITION, JIRA_SPLIT_TRANSITION to forge.jira.env.runner in harness/triage.yaml so the runner passes transition vars to the post-script - Revert docs/triage.md heading and terminology from "Multi-tracker" back to "Multi-forge" — the tracker/forge harness-split was rejected upstream (fullsend-ai/fullsend#6237) and the harness still uses forge: as the top-level key - Fix LOCAL.md Jira walkthrough to use FULLSEND_FORGE instead of FULLSEND_TRACKER - Add description fields to triage-result schema documenting the dual format for duplicate_of (integer vs Jira key) and repo (org/repo path vs bare Jira project key) - Remove .gitignore scope-creep additions (.claude, openspec) unrelated to Jira tracker support Addresses review feedback on fullsend-ai#827
Summary
overlays:approach proposed by @ifireball in the first review, replacing the original tracker/forge split direction.overlays:list field to the harness schema — each entry has a CELwhen:expression and the same override fields asForgeConfig. Multiple matching entries are merged in declaration order using ADR-0045 merge semantics.forge:(remains functional,Lint()warns) in favor ofoverlays:, which can condition on any event property rather than only the forge platform.docs/architecture.mdwith the corresponding Decided entry.Direction change after v1 review
The original PR proposed a
tracker:block sibling toforge:, keyed by platform. @ifireball's review pointed out that picking and choosing conditioning axes ends up feeling arbitrary, and proposed generalizing to CEL-guarded conditional sections instead. This rewrite adopts that direction —overlays:replaces both thetracker:proposal and (eventually)forge:itself.Test plan
make lintpasses (ADR frontmatter/status/number/link linters)🤖 Generated with Claude Code