OSAC-2870: rename 5 more EPs per Jira-content cross-check - #174
Merged
openshift-merge-bot[bot] merged 3 commits intoJul 30, 2026
Merged
openshift-merge-bot[bot] merged 3 commits into
openshift-merge-bot[bot] merged 3 commits into
Conversation
…comment thread) Follow-up to Eran Cohen's OSAC-2870 comment identifying resolvable Jira keys for 4 of the 8 remaining unrenamed directories, plus independent verification/correction of a 5th: - catalog-items -> OSAC-1002-catalog-items (NOT OSAC-1531 as originally suggested by directory-name similarity — verified via content: OSAC-1002's Jira description explicitly links PR osac-project#17, the exact PR that created this directory. OSAC-1531/PR osac-project#129's enhancements/OSAC-1531-default-catalog-items/ is a separate, later, narrower feature that assumes this catalog item API already exists — no actual duplication between the two.) - dns-api -> OSAC-1050-dns-api (dir + originating PR osac-project#29 both created 2026-03-17; problem statement nearly verbatim match; reporter Dan Manor = author dmanor) - organizations -> OSAC-1030-organizations (dir + originating PR osac-project#14 both created 2025-12-21; Jira explicitly links PR osac-project#14) - vm-api-fields -> OSAC-1034-vm-api-fields (dir + originating PR osac-project#21 both created 2026-01-27; parent Epic OSAC-61 authored by Michael Hrivnak = dir author mhrivnak) - repository-consolidation -> OSAC-1732-repository-consolidation (Epic-level key, not parent Feature OSAC-2053: OSAC-2053 is a broad 'CI Modernization & Quality' umbrella with 10 unrelated sibling Epics, while OSAC-1732's title is a word-for-word match of the EP's own title and Jira explicitly links PR osac-project#40, the exact originating PR. Also self-authored by Eran Cohen, who filed both.) Filled in tracking-link frontmatter for all 5 (previously empty/TBD). Updated 10 cross-references across 8 other files, including two hardcoded GitHub blob links in organizations/ui-design.md that pointed at the pre-rename path. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
@tchughesiv: This pull request references OSAC-2870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughUpdated Jira tracking metadata and internal enhancement links across catalog-items, organizations, metering, provisioning, image management, secret management, and related proposal documents. ChangesEnhancement reference updates
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 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 |
AI EP Review: EP-174Score: 2/10 | Verdict: FAIL
Verdict: This PR is a housekeeping change (directory renames and cross-reference updates) with no new product capability, no business justification, and no testable requirements — it does not belong in the PRD pipeline. Feedback: This work should be tracked as a Jira task, not submitted as a PRD or reviewed through the enhancement-proposal pipeline. PRDs describe new or changed product capabilities; directory renames and link cleanup are project maintenance. If you are submitting a new PRD alongside these renames, split them into separate PRs so the PRD content can be reviewed on its own merits. Critical (3)
Important (1)
Suggestions (1)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-174Score: 0/8 | Verdict: FAIL
Verdict: This PR is a directory-renaming housekeeping change, not a design document — it contains no design content to evaluate against the rubric, resulting in zeros across all criteria. Feedback: This PR standardizes enhancement proposal directory naming to include Jira keys and backfills tracking links, which is valuable maintenance work but is not a design document. The design review rubric does not apply to housekeeping PRs. If this PR is intended to accompany a design, the actual design content (Summary, Motivation, Proposal, API Extensions, Test Plan, etc.) needs to be included. Critical (1)
Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@enhancements/OSAC-1030-organizations/ui-design.md`:
- Line 7: Update the PRD table entry in ui-design.md so the displayed link label
matches its target URL, or change the URL to the actual PRD file; ensure the PRD
metadata is consistent for readers and tooling.
In `@enhancements/OSAC-1269-cluster-version-api/design.md`:
- Line 139: Update the catalog-items reference in the ClusterVersion design
document to point to the renamed enhancement directory using the correct
relative path, such as ../OSAC-1002-catalog-items/README.md, or the repository’s
canonical enhancement URL; leave the surrounding field-definition and validation
content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13f4a696-7fcd-4831-8d08-651a6fa6940a
📒 Files selected for processing (15)
enhancements/OSAC-1002-catalog-items/README.mdenhancements/OSAC-1002-catalog-items/ui-design.mdenhancements/OSAC-1030-organizations/README.mdenhancements/OSAC-1030-organizations/ui-design.mdenhancements/OSAC-1034-vm-api-fields/README.mdenhancements/OSAC-1050-dns-api/README.mdenhancements/OSAC-1118-baremetal-instance-api/README.mdenhancements/OSAC-1269-cluster-version-api/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.mdenhancements/OSAC-1567-secret-management/design.mdenhancements/OSAC-1732-repository-consolidation/README.mdenhancements/OSAC-979-image-management/README.mdenhancements/OSAC-985-metering-and-usage-tracking/design.mdenhancements/OSAC-985-metering-and-usage-tracking/prd.md
Full-repo sweep across all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) turned up two more stale references that earlier passes missed: - OSAC-1330-type-safe-resource-references/design.md still linked to '/enhancements/networking' (renamed to OSAC-356-networking in osac-project#144). This directory was self-renamed by osac-project#121's own branch, so it never went through our cross-reference sweep. - OSAC-985-metering-and-usage-tracking/design.md still linked to '/enhancements/vm-instance-types' (renamed to OSAC-46-vm-instance-types in osac-project#144). metering-and-usage-tracking was one of the directories deferred at that time due to an open PR, so it was excluded from that pass's cross-reference sweep and the reference went stale once the deferred rename landed in osac-project#149. No open PRs conflict with either file (re-verified). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
AI EP Review: EP-174Score: 1/10 | Verdict: FAIL
Verdict: This PR is a directory-renaming housekeeping change, not a PRD — it introduces no new product capability, making it unsuitable for the enhancement proposal pipeline. Feedback: This PR renames enhancement directories to follow the OSAC-NNNN-slug naming convention and backfills tracking links — both valuable housekeeping tasks. However, they should be tracked as a Jira task, not submitted through the PRD/enhancement pipeline. No PRD review criteria can meaningfully apply to file reorganization work. Critical (1)
Important (0)None. Suggestions (1)
Review costModel: claude-opus-4-6 |
- OSAC-1030-organizations/ui-design.md: fix PRD table link label
('03-prd.md' -> 'README.md') to match its target file. Pre-existing
mismatch, unrelated to the directory rename itself.
- OSAC-1269-cluster-version-api/design.md: simplify the catalog-items
EP link from a broken '../../../enhancement-proposals/enhancements/...'
roundtrip (only happens to resolve in a local osac-workspace
multi-repo checkout, not on GitHub.com) to the correct same-repo
relative path '../OSAC-1002-catalog-items/README.md'. Pre-existing
bug inherited via find-replace on the directory name, not introduced
by this PR.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Tommy Hughes <tohughes@redhat.com>
AI Design Review: EP-174Score: 0/8 | Verdict: FAIL
Verdict: This PR is a housekeeping/maintenance change that renames enhancement directories to the OSAC-- convention and updates cross-references — it contains no design document content and cannot pass a design review rubric. Feedback: This PR is not a design document and should not be reviewed against the design document rubric. It is a valid maintenance PR that standardizes directory naming (e.g., catalog-items/ → OSAC-1002-catalog-items/) and replaces TBD tracking links with actual Jira URLs. If this PR is intended to accompany a design, the actual design content should be included. As a standalone housekeeping change, it should be reviewed for link correctness and completeness rather than design quality. Critical (1)
Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-174Score: 0/10 | Verdict: FAIL
Verdict: This PR is a housekeeping task (directory renames and link fixes), not a PRD — it introduces no new product capability and should be tracked as a Jira task, not reviewed through the PRD pipeline. Feedback: This PR renames enhancement directories to the OSAC-- convention and updates cross-references, which is valuable maintenance work but not a PRD. Track it as a standard Jira task and merge it through normal code review. The PRD review process is for proposals that introduce new user-facing capabilities. Critical (1)
Important (0)None. Suggestions (0)None. Review costModel: claude-opus-4-6 |
AI Design Review: EP-174Score: 0/8 | Verdict: FAIL
Verdict: This PR is not a design document — it is an administrative/housekeeping change that renames enhancement directories to the OSAC-- convention and fixes tracking links, so the design review rubric does not apply. Feedback: This PR does useful work (standardizing directory names, replacing TBD tracking links with real Jira URLs, updating cross-references), but it is not a design document and should not be evaluated against the design review rubric. If this PR is submitted alongside or as part of a design proposal, the actual design content should be included in the diff for review. As a standalone housekeeping PR, it would benefit from a checklist verifying that all cross-references were updated (grep for old directory names to catch any missed references). Critical (1)
Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
Contributor
|
/lgtm |
avishayt
approved these changes
Jul 30, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avishayt, eranco74, tchughesiv The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 2, 2026
- OSAC-1030-organizations/ui-design.md: fix PRD table link label
('03-prd.md' -> 'README.md') to match its target file. Pre-existing
mismatch, unrelated to the directory rename itself.
- OSAC-1269-cluster-version-api/design.md: simplify the catalog-items
EP link from a broken '../../../enhancement-proposals/enhancements/...'
roundtrip (only happens to resolve in a local osac-workspace
multi-repo checkout, not on GitHub.com) to the correct same-repo
relative path '../OSAC-1002-catalog-items/README.md'. Pre-existing
bug inherited via find-replace on the directory name, not introduced
by this PR.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Tommy Hughes <tohughes@redhat.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…_LOGISTICS Adds ep_classify.classify_logistics_only(), a per-file diff classifier that recognizes three provably-safe shapes (pure rename, allow-listed frontmatter field change, same-line link/path substitution) and falls back to SUBSTANTIVE for anything else — "when in doubt, review." Widens get_changed_files() to fetch status/previous_filename/patch per file (Phase A's filename-only consumers are unaffected via filenames_only()). Ships default-off: EP_REVIEW_SKIP_LOGISTICS=false means the classifier runs and logs its verdict against every real PR, but the full review pipeline always still runs — pure observability during burn-in, per the frozen design's rollout plan. Wired the flag into ep-review.yml's vars.EP_REVIEW_SKIP_LOGISTICS the same way EP_REVIEW_SHADOW already is, so flipping it later needs only a repo variable change. When flipped on, a LOGISTICS_ONLY verdict skips run_review() and posts hooks.apply_logistics_comment()'s minimal comment instead. Corrects a contradiction found in the frozen local design while implementing it: an earlier draft forced SUBSTANTIVE whenever Phase A's derive_feature_key reported PR-wide key ambiguity, which would have wrongly failed PR osac-project#174 (a legitimate 16-file bulk rename touching 5 distinct Feature keys). Feature-key attribution and logistics classification are separate concerns — classify_logistics_only doesn't call derive_feature_key at all, and decides purely per-file. See the "Correction" note added to .planning/OSAC-3416_DESIGN.md. Golden-fixture tests (testdata/pr{168,172,173,174}_files.json, captured from the real GitHub API) confirm osac-project#174 -> LOGISTICS_ONLY and osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE, including osac-project#172's frontmatter-plus-prose- rewrite case that guards against a naive "any frontmatter touched => skip" bug. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
Recovered findings from an interrupted `/code-review high` run on the logistics-classifier work (commit 12ed080), each verified against actual code/dependencies before fixing: 1. ticket never reached the real prompt/comment. agentic_ci.skill.run_skill only forwards its `ticket=` argument to pre_gates/context_writer/ extension_config_writer — prompt_builder and label_applier only ever see **extra_kwargs, which excludes `ticket` since it's a named parameter of run_skill(), not a passthrough kwarg. Confirmed against both the installed agentic-ci 0.3.37 and the repo's floor (0.3.15): identical behavior in both. In production this meant build_prompt()/apply_labels() always rendered "Jira Feature key: could not be determined" and empty structural notes, even when derive_feature_key() worked correctly. Fixed at the ep_skill_config.py seam: prompt_builder/label_applier are now wrapped to rename a second `osac_ticket=` kwarg (which *does* survive into **extra_kwargs) back to `ticket`, so ep_hooks.py's public method signatures are untouched. Added test_ep_review.py::RunReviewRealSeamTests, which drives the real agentic_ci.skill.run_skill (container execution faked out) instead of calling EPHooks.build_prompt()/apply_labels() directly — proven to fail against the pre-fix code. 2. Frontmatter false positive in ep_classify._hunk_is_safe. An allow-listed field (e.g. last-updated) edited earlier in a hunk left its field name "sticky" for the rest of the hunk, so an unrelated real content change later in the *same* hunk — even past the closing `---` — was wrongly treated as another safe frontmatter edit. Fixed by resetting the tracked field at the frontmatter delimiter. Verified the fix doesn't regress the legitimate multi-line YAML continuation shapes real PR osac-project#174 relies on (e.g. `tracking-link:\n - <url>`), which need the field-tracking to survive across a value's continuation lines. 3. Logistics-only path skipped same-SHA dedup. The EP_REVIEW_SKIP_LOGISTICS short-circuit calls hooks.apply_logistics_comment() directly, bypassing the check_pr_state() pre-gate that the full-review path gets for free via run_skill()'s pre_gates list — a rerun at an unchanged head SHA could post a duplicate logistics comment. Fixed by calling check_pr_state() explicitly before posting, reusing the existing dedup logic (no duplicated marker/SHA matching). 4. Rename into a canonical doc wasn't forced SUBSTANTIVE. The "a brand-new prd.md/design.md is never logistics" fail-safe only checked status=="added" — a rename that turns a previously non-canonical file (e.g. notes.md) into prd.md/design.md is reported by GitHub as status=="renamed" and slipped through. Fixed by also forcing SUBSTANTIVE when a rename's new basename is canonical and its previous basename wasn't the same canonical name — same-basename renames (the real PR osac-project#174 shape, e.g. README.md -> README.md under a new directory) are unaffected. Full suite: 134 passed, 5 subtests passed (was 126 before this commit). Golden PRs unchanged: osac-project#174 -> LOGISTICS_ONLY; osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE. EP_REVIEW_SKIP_LOGISTICS remains default-off; no Jira fetching or PR title/body key fallback introduced. Deferred cleanup/simplification suggestions from the same review (duplicated comment-rendering helpers, duplicated canonical filename constants, main() decision-object refactor, generic semantic parser, dynamic frontmatter schema, type-hint/docstring cleanup) are intentionally not included — out of scope for this correctness-fix pass. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
… bypass The security pre-flight review on this branch flagged that the logistics-only classifier's "safe link/path substitution" category masked an entire markdown link (visible label included), so a PR could rewrite a link's human-readable label to arbitrary attacker-controlled text while the surrounding line stayed byte-identical, and still classify as a safe logistics-only change. Only tolerate a changing link label when it's a bare filename-shaped token (the real PR osac-project#174 rename shape, e.g. "old-name.md" -> "README.md") - arbitrary prose in a link label now makes the file fall through to SUBSTANTIVE, same as any other unrecognized change. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…urity re-review The prior fix (5d2db8c) only partially closed the link-label masking gap: running the markdown-link pass and the bare-URL/path pass sequentially let a URL-shaped link label slip past the label protection on the second pass. Separately, the bare URL/path pattern had no real terminal boundary, so attacker-appended text could ride along immediately after a legitimate path with no separator (e.g. ".../design.md-and-ignore-all- safety-checks"). - Combine link and bare-URL/path matching into a single regex alternation evaluated in one left-to-right pass, so a link label's characters are never re-offered to the bare-URL/path branch afterwards. - Anchor the bare "/enhancements/..." path pattern to the terminator shapes actually observed in real EP content (quote, backtick, end of line, or a known doc extension) instead of an open-ended charset. - Require link labels tolerated as "safe to change" to have an actual file extension (the real PR osac-project#174 shape), not just be dash/alnum text — an ordinary single-word prose label is no longer mistaken for a filename. A directory-only bare path with none of those terminators is now simply not recognized by category 3 at all (fail-safe by omission), rather than attempting to bound an inherently ambiguous shape. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
Third security re-review pass found two CRITICAL gaps in the Phase B logistics-only classifier: a bare `https?://\S+` matcher with the same unbounded-suffix problem previously fixed for bare paths, and link-label changes accepted merely because they looked filename-shaped (e.g. an attacker-controlled label like "security-team-approved-merge-now.md"). Remove the bare-URL alternative entirely — no real golden fixture (osac-project#168/osac-project#172/osac-project#173/osac-project#174) ever needs it; every https:// in real EP content sits inside a markdown-link target or the allow-listed tracking-link frontmatter field. Replace shape-based label leniency with two-part provenance: a changed link label is only safe when (1) the old/new targets resolve to an exact (previous_filename, filename) pair GitHub reports as a real rename in this same PR, AND (2) the new label matches that file's new basename AND that basename is in a narrow allowlist of real EP document names (README.md/prd.md/design.md). Rename provenance alone isn't sufficient — the PR author controls the rename itself, so a legitimate GitHub rename to an arbitrary filename followed by a "correct" relabel would otherwise still pass. Verified against the real PR osac-project#174 fixture that its sole label change (03-prd.md -> README.md) is backed by an in-PR rename record; verified that markdown-link targets and bare "/enhancements/..." paths must stay provenance-free, since osac-project#174 itself contains cross-references to renames from earlier PRs with no corroborating record in this PR's own file list. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…m list Security preflight found that the "does this introduce a first-seen reviewable EP identity" guard only special-cased status "added" and "renamed", leaving GitHub's documented "copied" status (which can introduce a first-seen reviewable path, e.g. via a copy with only a narrow-safe tracking-link patch) as an unguarded bypass. Replace the two ad hoc status checks with a structural invariant: _introduces_new_reviewable_identity() treats "modified" (and a same-basename "renamed", the PR osac-project#174 shape) as the only identity-preserving transitions, and forces SUBSTANTIVE for every other status — added, copied, changed, unchanged, removed, or any future status this enum grows — when the basename is reviewable. This closes the whole class in one pass instead of re-enumerating one status at a time each review round. Also import ep_paths.CANONICAL_FILENAMES directly instead of maintaining an independent literal copy, per the review's non-blocking drift-risk note — the two constants can no longer silently diverge. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
6 tasks done
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…_LOGISTICS Adds ep_classify.classify_logistics_only(), a per-file diff classifier that recognizes three provably-safe shapes (pure rename, allow-listed frontmatter field change, same-line link/path substitution) and falls back to SUBSTANTIVE for anything else — "when in doubt, review." Widens get_changed_files() to fetch status/previous_filename/patch per file (Phase A's filename-only consumers are unaffected via filenames_only()). Ships default-off: EP_REVIEW_SKIP_LOGISTICS=false means the classifier runs and logs its verdict against every real PR, but the full review pipeline always still runs — pure observability during burn-in, per the frozen design's rollout plan. Wired the flag into ep-review.yml's vars.EP_REVIEW_SKIP_LOGISTICS the same way EP_REVIEW_SHADOW already is, so flipping it later needs only a repo variable change. When flipped on, a LOGISTICS_ONLY verdict skips run_review() and posts hooks.apply_logistics_comment()'s minimal comment instead. Corrects a contradiction found in the frozen local design while implementing it: an earlier draft forced SUBSTANTIVE whenever Phase A's derive_feature_key reported PR-wide key ambiguity, which would have wrongly failed PR osac-project#174 (a legitimate 16-file bulk rename touching 5 distinct Feature keys). Feature-key attribution and logistics classification are separate concerns — classify_logistics_only doesn't call derive_feature_key at all, and decides purely per-file. See the "Correction" note added to .planning/OSAC-3416_DESIGN.md. Golden-fixture tests (testdata/pr{168,172,173,174}_files.json, captured from the real GitHub API) confirm osac-project#174 -> LOGISTICS_ONLY and osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE, including osac-project#172's frontmatter-plus-prose- rewrite case that guards against a naive "any frontmatter touched => skip" bug. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
Recovered findings from an interrupted `/code-review high` run on the logistics-classifier work (commit 12ed080), each verified against actual code/dependencies before fixing: 1. ticket never reached the real prompt/comment. agentic_ci.skill.run_skill only forwards its `ticket=` argument to pre_gates/context_writer/ extension_config_writer — prompt_builder and label_applier only ever see **extra_kwargs, which excludes `ticket` since it's a named parameter of run_skill(), not a passthrough kwarg. Confirmed against both the installed agentic-ci 0.3.37 and the repo's floor (0.3.15): identical behavior in both. In production this meant build_prompt()/apply_labels() always rendered "Jira Feature key: could not be determined" and empty structural notes, even when derive_feature_key() worked correctly. Fixed at the ep_skill_config.py seam: prompt_builder/label_applier are now wrapped to rename a second `osac_ticket=` kwarg (which *does* survive into **extra_kwargs) back to `ticket`, so ep_hooks.py's public method signatures are untouched. Added test_ep_review.py::RunReviewRealSeamTests, which drives the real agentic_ci.skill.run_skill (container execution faked out) instead of calling EPHooks.build_prompt()/apply_labels() directly — proven to fail against the pre-fix code. 2. Frontmatter false positive in ep_classify._hunk_is_safe. An allow-listed field (e.g. last-updated) edited earlier in a hunk left its field name "sticky" for the rest of the hunk, so an unrelated real content change later in the *same* hunk — even past the closing `---` — was wrongly treated as another safe frontmatter edit. Fixed by resetting the tracked field at the frontmatter delimiter. Verified the fix doesn't regress the legitimate multi-line YAML continuation shapes real PR osac-project#174 relies on (e.g. `tracking-link:\n - <url>`), which need the field-tracking to survive across a value's continuation lines. 3. Logistics-only path skipped same-SHA dedup. The EP_REVIEW_SKIP_LOGISTICS short-circuit calls hooks.apply_logistics_comment() directly, bypassing the check_pr_state() pre-gate that the full-review path gets for free via run_skill()'s pre_gates list — a rerun at an unchanged head SHA could post a duplicate logistics comment. Fixed by calling check_pr_state() explicitly before posting, reusing the existing dedup logic (no duplicated marker/SHA matching). 4. Rename into a canonical doc wasn't forced SUBSTANTIVE. The "a brand-new prd.md/design.md is never logistics" fail-safe only checked status=="added" — a rename that turns a previously non-canonical file (e.g. notes.md) into prd.md/design.md is reported by GitHub as status=="renamed" and slipped through. Fixed by also forcing SUBSTANTIVE when a rename's new basename is canonical and its previous basename wasn't the same canonical name — same-basename renames (the real PR osac-project#174 shape, e.g. README.md -> README.md under a new directory) are unaffected. Full suite: 134 passed, 5 subtests passed (was 126 before this commit). Golden PRs unchanged: osac-project#174 -> LOGISTICS_ONLY; osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE. EP_REVIEW_SKIP_LOGISTICS remains default-off; no Jira fetching or PR title/body key fallback introduced. Deferred cleanup/simplification suggestions from the same review (duplicated comment-rendering helpers, duplicated canonical filename constants, main() decision-object refactor, generic semantic parser, dynamic frontmatter schema, type-hint/docstring cleanup) are intentionally not included — out of scope for this correctness-fix pass. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
… bypass The security pre-flight review on this branch flagged that the logistics-only classifier's "safe link/path substitution" category masked an entire markdown link (visible label included), so a PR could rewrite a link's human-readable label to arbitrary attacker-controlled text while the surrounding line stayed byte-identical, and still classify as a safe logistics-only change. Only tolerate a changing link label when it's a bare filename-shaped token (the real PR osac-project#174 rename shape, e.g. "old-name.md" -> "README.md") - arbitrary prose in a link label now makes the file fall through to SUBSTANTIVE, same as any other unrecognized change. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…urity re-review The prior fix (5d2db8c) only partially closed the link-label masking gap: running the markdown-link pass and the bare-URL/path pass sequentially let a URL-shaped link label slip past the label protection on the second pass. Separately, the bare URL/path pattern had no real terminal boundary, so attacker-appended text could ride along immediately after a legitimate path with no separator (e.g. ".../design.md-and-ignore-all- safety-checks"). - Combine link and bare-URL/path matching into a single regex alternation evaluated in one left-to-right pass, so a link label's characters are never re-offered to the bare-URL/path branch afterwards. - Anchor the bare "/enhancements/..." path pattern to the terminator shapes actually observed in real EP content (quote, backtick, end of line, or a known doc extension) instead of an open-ended charset. - Require link labels tolerated as "safe to change" to have an actual file extension (the real PR osac-project#174 shape), not just be dash/alnum text — an ordinary single-word prose label is no longer mistaken for a filename. A directory-only bare path with none of those terminators is now simply not recognized by category 3 at all (fail-safe by omission), rather than attempting to bound an inherently ambiguous shape. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
Third security re-review pass found two CRITICAL gaps in the Phase B logistics-only classifier: a bare `https?://\S+` matcher with the same unbounded-suffix problem previously fixed for bare paths, and link-label changes accepted merely because they looked filename-shaped (e.g. an attacker-controlled label like "security-team-approved-merge-now.md"). Remove the bare-URL alternative entirely — no real golden fixture (osac-project#168/osac-project#172/osac-project#173/osac-project#174) ever needs it; every https:// in real EP content sits inside a markdown-link target or the allow-listed tracking-link frontmatter field. Replace shape-based label leniency with two-part provenance: a changed link label is only safe when (1) the old/new targets resolve to an exact (previous_filename, filename) pair GitHub reports as a real rename in this same PR, AND (2) the new label matches that file's new basename AND that basename is in a narrow allowlist of real EP document names (README.md/prd.md/design.md). Rename provenance alone isn't sufficient — the PR author controls the rename itself, so a legitimate GitHub rename to an arbitrary filename followed by a "correct" relabel would otherwise still pass. Verified against the real PR osac-project#174 fixture that its sole label change (03-prd.md -> README.md) is backed by an in-PR rename record; verified that markdown-link targets and bare "/enhancements/..." paths must stay provenance-free, since osac-project#174 itself contains cross-references to renames from earlier PRs with no corroborating record in this PR's own file list. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…m list Security preflight found that the "does this introduce a first-seen reviewable EP identity" guard only special-cased status "added" and "renamed", leaving GitHub's documented "copied" status (which can introduce a first-seen reviewable path, e.g. via a copy with only a narrow-safe tracking-link patch) as an unguarded bypass. Replace the two ad hoc status checks with a structural invariant: _introduces_new_reviewable_identity() treats "modified" (and a same-basename "renamed", the PR osac-project#174 shape) as the only identity-preserving transitions, and forces SUBSTANTIVE for every other status — added, copied, changed, unchanged, removed, or any future status this enum grows — when the basename is reviewable. This closes the whole class in one pass instead of re-enumerating one status at a time each review round. Also import ep_paths.CANONICAL_FILENAMES directly instead of maintaining an independent literal copy, per the review's non-blocking drift-risk note — the two constants can no longer silently diverge. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
…the final field PR review (ItzikEzra-rh) found HIGH: the allow-listed-frontmatter shortcut in _hunk_is_safe decided from the change group's FINAL field state only, via _update_frontmatter_field's context-line-oriented leniency (it leaves the field unchanged for any line that isn't itself a recognized `key:` or the `---` delimiter, to support real YAML continuations like ` - value`). That same leniency let an unrelated, unindented prose line silently "inherit" whatever field was last tracked, so a contiguous +/- group (no separating context line) ending on an allow-listed last-updated/tracking-link line waved through arbitrary injected or deleted prose earlier in the same group. Track a sticky group_all_safe flag and verify every changed line individually: a continuation is only trusted when it's indented (real YAML list/wrapped-value shape) AND a field is already tracked; anything else (unindented, non-key, non-delimiter) breaks the chain to None, which can never be allow-listed. Sticky means a later line re-establishing a valid key can't retroactively excuse an earlier unsafe line in the same group. Legitimate multi-line YAML continuation behavior (PR osac-project#174's tracking-link list-item edits) is unaffected, since those continuation lines are indented and never break the chain. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 20, 2026
PR review (tchughesiv) noted a remaining nit: markdown-link targets were treated as "always safe to change" even when the label stayed identical, so a same-label retarget to an arbitrary external URL classified LOGISTICS_ONLY unconditionally. The target isn't rendered as visible text, but it's still where a reader ends up navigating to, so an external retarget is its own attacker-controlled-destination risk independent of the label check. Add _target_is_repo_local: whenever a markdown-link target actually changes, both the old and new target must be one of the shapes real PR osac-project#174 evidence needs — a bare "/enhancements/..." path, a GitHub blob URL for this exact repo, or a relative repo path with no URI scheme and no other absolute-path prefix. An unchanged target is never re-validated, so an already-external link elsewhere on the same line (e.g. osac-project#174's cross-repo applyFieldDefinitions() link) is unaffected. Rename-pair provenance for labels is unchanged; this is an additional, independent check specifically for the target's destination. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
…_LOGISTICS Adds ep_classify.classify_logistics_only(), a per-file diff classifier that recognizes three provably-safe shapes (pure rename, allow-listed frontmatter field change, same-line link/path substitution) and falls back to SUBSTANTIVE for anything else — "when in doubt, review." Widens get_changed_files() to fetch status/previous_filename/patch per file (Phase A's filename-only consumers are unaffected via filenames_only()). Ships default-off: EP_REVIEW_SKIP_LOGISTICS=false means the classifier runs and logs its verdict against every real PR, but the full review pipeline always still runs — pure observability during burn-in, per the frozen design's rollout plan. Wired the flag into ep-review.yml's vars.EP_REVIEW_SKIP_LOGISTICS the same way EP_REVIEW_SHADOW already is, so flipping it later needs only a repo variable change. When flipped on, a LOGISTICS_ONLY verdict skips run_review() and posts hooks.apply_logistics_comment()'s minimal comment instead. Corrects a contradiction found in the frozen local design while implementing it: an earlier draft forced SUBSTANTIVE whenever Phase A's derive_feature_key reported PR-wide key ambiguity, which would have wrongly failed PR osac-project#174 (a legitimate 16-file bulk rename touching 5 distinct Feature keys). Feature-key attribution and logistics classification are separate concerns — classify_logistics_only doesn't call derive_feature_key at all, and decides purely per-file. See the "Correction" note added to .planning/OSAC-3416_DESIGN.md. Golden-fixture tests (testdata/pr{168,172,173,174}_files.json, captured from the real GitHub API) confirm osac-project#174 -> LOGISTICS_ONLY and osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE, including osac-project#172's frontmatter-plus-prose- rewrite case that guards against a naive "any frontmatter touched => skip" bug. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
Recovered findings from an interrupted `/code-review high` run on the logistics-classifier work (commit 12ed080), each verified against actual code/dependencies before fixing: 1. ticket never reached the real prompt/comment. agentic_ci.skill.run_skill only forwards its `ticket=` argument to pre_gates/context_writer/ extension_config_writer — prompt_builder and label_applier only ever see **extra_kwargs, which excludes `ticket` since it's a named parameter of run_skill(), not a passthrough kwarg. Confirmed against both the installed agentic-ci 0.3.37 and the repo's floor (0.3.15): identical behavior in both. In production this meant build_prompt()/apply_labels() always rendered "Jira Feature key: could not be determined" and empty structural notes, even when derive_feature_key() worked correctly. Fixed at the ep_skill_config.py seam: prompt_builder/label_applier are now wrapped to rename a second `osac_ticket=` kwarg (which *does* survive into **extra_kwargs) back to `ticket`, so ep_hooks.py's public method signatures are untouched. Added test_ep_review.py::RunReviewRealSeamTests, which drives the real agentic_ci.skill.run_skill (container execution faked out) instead of calling EPHooks.build_prompt()/apply_labels() directly — proven to fail against the pre-fix code. 2. Frontmatter false positive in ep_classify._hunk_is_safe. An allow-listed field (e.g. last-updated) edited earlier in a hunk left its field name "sticky" for the rest of the hunk, so an unrelated real content change later in the *same* hunk — even past the closing `---` — was wrongly treated as another safe frontmatter edit. Fixed by resetting the tracked field at the frontmatter delimiter. Verified the fix doesn't regress the legitimate multi-line YAML continuation shapes real PR osac-project#174 relies on (e.g. `tracking-link:\n - <url>`), which need the field-tracking to survive across a value's continuation lines. 3. Logistics-only path skipped same-SHA dedup. The EP_REVIEW_SKIP_LOGISTICS short-circuit calls hooks.apply_logistics_comment() directly, bypassing the check_pr_state() pre-gate that the full-review path gets for free via run_skill()'s pre_gates list — a rerun at an unchanged head SHA could post a duplicate logistics comment. Fixed by calling check_pr_state() explicitly before posting, reusing the existing dedup logic (no duplicated marker/SHA matching). 4. Rename into a canonical doc wasn't forced SUBSTANTIVE. The "a brand-new prd.md/design.md is never logistics" fail-safe only checked status=="added" — a rename that turns a previously non-canonical file (e.g. notes.md) into prd.md/design.md is reported by GitHub as status=="renamed" and slipped through. Fixed by also forcing SUBSTANTIVE when a rename's new basename is canonical and its previous basename wasn't the same canonical name — same-basename renames (the real PR osac-project#174 shape, e.g. README.md -> README.md under a new directory) are unaffected. Full suite: 134 passed, 5 subtests passed (was 126 before this commit). Golden PRs unchanged: osac-project#174 -> LOGISTICS_ONLY; osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE. EP_REVIEW_SKIP_LOGISTICS remains default-off; no Jira fetching or PR title/body key fallback introduced. Deferred cleanup/simplification suggestions from the same review (duplicated comment-rendering helpers, duplicated canonical filename constants, main() decision-object refactor, generic semantic parser, dynamic frontmatter schema, type-hint/docstring cleanup) are intentionally not included — out of scope for this correctness-fix pass. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
… bypass The security pre-flight review on this branch flagged that the logistics-only classifier's "safe link/path substitution" category masked an entire markdown link (visible label included), so a PR could rewrite a link's human-readable label to arbitrary attacker-controlled text while the surrounding line stayed byte-identical, and still classify as a safe logistics-only change. Only tolerate a changing link label when it's a bare filename-shaped token (the real PR osac-project#174 rename shape, e.g. "old-name.md" -> "README.md") - arbitrary prose in a link label now makes the file fall through to SUBSTANTIVE, same as any other unrecognized change. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
…urity re-review The prior fix (5d2db8c) only partially closed the link-label masking gap: running the markdown-link pass and the bare-URL/path pass sequentially let a URL-shaped link label slip past the label protection on the second pass. Separately, the bare URL/path pattern had no real terminal boundary, so attacker-appended text could ride along immediately after a legitimate path with no separator (e.g. ".../design.md-and-ignore-all- safety-checks"). - Combine link and bare-URL/path matching into a single regex alternation evaluated in one left-to-right pass, so a link label's characters are never re-offered to the bare-URL/path branch afterwards. - Anchor the bare "/enhancements/..." path pattern to the terminator shapes actually observed in real EP content (quote, backtick, end of line, or a known doc extension) instead of an open-ended charset. - Require link labels tolerated as "safe to change" to have an actual file extension (the real PR osac-project#174 shape), not just be dash/alnum text — an ordinary single-word prose label is no longer mistaken for a filename. A directory-only bare path with none of those terminators is now simply not recognized by category 3 at all (fail-safe by omission), rather than attempting to bound an inherently ambiguous shape. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
Third security re-review pass found two CRITICAL gaps in the Phase B logistics-only classifier: a bare `https?://\S+` matcher with the same unbounded-suffix problem previously fixed for bare paths, and link-label changes accepted merely because they looked filename-shaped (e.g. an attacker-controlled label like "security-team-approved-merge-now.md"). Remove the bare-URL alternative entirely — no real golden fixture (osac-project#168/osac-project#172/osac-project#173/osac-project#174) ever needs it; every https:// in real EP content sits inside a markdown-link target or the allow-listed tracking-link frontmatter field. Replace shape-based label leniency with two-part provenance: a changed link label is only safe when (1) the old/new targets resolve to an exact (previous_filename, filename) pair GitHub reports as a real rename in this same PR, AND (2) the new label matches that file's new basename AND that basename is in a narrow allowlist of real EP document names (README.md/prd.md/design.md). Rename provenance alone isn't sufficient — the PR author controls the rename itself, so a legitimate GitHub rename to an arbitrary filename followed by a "correct" relabel would otherwise still pass. Verified against the real PR osac-project#174 fixture that its sole label change (03-prd.md -> README.md) is backed by an in-PR rename record; verified that markdown-link targets and bare "/enhancements/..." paths must stay provenance-free, since osac-project#174 itself contains cross-references to renames from earlier PRs with no corroborating record in this PR's own file list. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
…m list Security preflight found that the "does this introduce a first-seen reviewable EP identity" guard only special-cased status "added" and "renamed", leaving GitHub's documented "copied" status (which can introduce a first-seen reviewable path, e.g. via a copy with only a narrow-safe tracking-link patch) as an unguarded bypass. Replace the two ad hoc status checks with a structural invariant: _introduces_new_reviewable_identity() treats "modified" (and a same-basename "renamed", the PR osac-project#174 shape) as the only identity-preserving transitions, and forces SUBSTANTIVE for every other status — added, copied, changed, unchanged, removed, or any future status this enum grows — when the basename is reviewable. This closes the whole class in one pass instead of re-enumerating one status at a time each review round. Also import ep_paths.CANONICAL_FILENAMES directly instead of maintaining an independent literal copy, per the review's non-blocking drift-risk note — the two constants can no longer silently diverge. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
…the final field PR review (ItzikEzra-rh) found HIGH: the allow-listed-frontmatter shortcut in _hunk_is_safe decided from the change group's FINAL field state only, via _update_frontmatter_field's context-line-oriented leniency (it leaves the field unchanged for any line that isn't itself a recognized `key:` or the `---` delimiter, to support real YAML continuations like ` - value`). That same leniency let an unrelated, unindented prose line silently "inherit" whatever field was last tracked, so a contiguous +/- group (no separating context line) ending on an allow-listed last-updated/tracking-link line waved through arbitrary injected or deleted prose earlier in the same group. Track a sticky group_all_safe flag and verify every changed line individually: a continuation is only trusted when it's indented (real YAML list/wrapped-value shape) AND a field is already tracked; anything else (unindented, non-key, non-delimiter) breaks the chain to None, which can never be allow-listed. Sticky means a later line re-establishing a valid key can't retroactively excuse an earlier unsafe line in the same group. Legitimate multi-line YAML continuation behavior (PR osac-project#174's tracking-link list-item edits) is unaffected, since those continuation lines are indented and never break the chain. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
omerough
added a commit
to omerough/enhancement-proposals
that referenced
this pull request
Aug 25, 2026
PR review (tchughesiv) noted a remaining nit: markdown-link targets were treated as "always safe to change" even when the label stayed identical, so a same-label retarget to an arbitrary external URL classified LOGISTICS_ONLY unconditionally. The target isn't rendered as visible text, but it's still where a reader ends up navigating to, so an external retarget is its own attacker-controlled-destination risk independent of the label check. Add _target_is_repo_local: whenever a markdown-link target actually changes, both the old and new target must be one of the shapes real PR osac-project#174 evidence needs — a bare "/enhancements/..." path, a GitHub blob URL for this exact repo, or a relative repo path with no URI scheme and no other absolute-path prefix. An unchanged target is never re-validated, so an already-external link elsewhere on the same line (e.g. osac-project#174's cross-repo applyFieldDefinitions() link) is unaffected. Rename-pair provenance for labels is unchanged; this is an additional, independent check specifically for the target's destination. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
…_LOGISTICS Adds ep_classify.classify_logistics_only(), a per-file diff classifier that recognizes three provably-safe shapes (pure rename, allow-listed frontmatter field change, same-line link/path substitution) and falls back to SUBSTANTIVE for anything else — "when in doubt, review." Widens get_changed_files() to fetch status/previous_filename/patch per file (Phase A's filename-only consumers are unaffected via filenames_only()). Ships default-off: EP_REVIEW_SKIP_LOGISTICS=false means the classifier runs and logs its verdict against every real PR, but the full review pipeline always still runs — pure observability during burn-in, per the frozen design's rollout plan. Wired the flag into ep-review.yml's vars.EP_REVIEW_SKIP_LOGISTICS the same way EP_REVIEW_SHADOW already is, so flipping it later needs only a repo variable change. When flipped on, a LOGISTICS_ONLY verdict skips run_review() and posts hooks.apply_logistics_comment()'s minimal comment instead. Corrects a contradiction found in the frozen local design while implementing it: an earlier draft forced SUBSTANTIVE whenever Phase A's derive_feature_key reported PR-wide key ambiguity, which would have wrongly failed PR osac-project#174 (a legitimate 16-file bulk rename touching 5 distinct Feature keys). Feature-key attribution and logistics classification are separate concerns — classify_logistics_only doesn't call derive_feature_key at all, and decides purely per-file. See the "Correction" note added to .planning/OSAC-3416_DESIGN.md. Golden-fixture tests (testdata/pr{168,172,173,174}_files.json, captured from the real GitHub API) confirm osac-project#174 -> LOGISTICS_ONLY and osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE, including osac-project#172's frontmatter-plus-prose- rewrite case that guards against a naive "any frontmatter touched => skip" bug. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
Recovered findings from an interrupted `/code-review high` run on the logistics-classifier work (commit 12ed080), each verified against actual code/dependencies before fixing: 1. ticket never reached the real prompt/comment. agentic_ci.skill.run_skill only forwards its `ticket=` argument to pre_gates/context_writer/ extension_config_writer — prompt_builder and label_applier only ever see **extra_kwargs, which excludes `ticket` since it's a named parameter of run_skill(), not a passthrough kwarg. Confirmed against both the installed agentic-ci 0.3.37 and the repo's floor (0.3.15): identical behavior in both. In production this meant build_prompt()/apply_labels() always rendered "Jira Feature key: could not be determined" and empty structural notes, even when derive_feature_key() worked correctly. Fixed at the ep_skill_config.py seam: prompt_builder/label_applier are now wrapped to rename a second `osac_ticket=` kwarg (which *does* survive into **extra_kwargs) back to `ticket`, so ep_hooks.py's public method signatures are untouched. Added test_ep_review.py::RunReviewRealSeamTests, which drives the real agentic_ci.skill.run_skill (container execution faked out) instead of calling EPHooks.build_prompt()/apply_labels() directly — proven to fail against the pre-fix code. 2. Frontmatter false positive in ep_classify._hunk_is_safe. An allow-listed field (e.g. last-updated) edited earlier in a hunk left its field name "sticky" for the rest of the hunk, so an unrelated real content change later in the *same* hunk — even past the closing `---` — was wrongly treated as another safe frontmatter edit. Fixed by resetting the tracked field at the frontmatter delimiter. Verified the fix doesn't regress the legitimate multi-line YAML continuation shapes real PR osac-project#174 relies on (e.g. `tracking-link:\n - <url>`), which need the field-tracking to survive across a value's continuation lines. 3. Logistics-only path skipped same-SHA dedup. The EP_REVIEW_SKIP_LOGISTICS short-circuit calls hooks.apply_logistics_comment() directly, bypassing the check_pr_state() pre-gate that the full-review path gets for free via run_skill()'s pre_gates list — a rerun at an unchanged head SHA could post a duplicate logistics comment. Fixed by calling check_pr_state() explicitly before posting, reusing the existing dedup logic (no duplicated marker/SHA matching). 4. Rename into a canonical doc wasn't forced SUBSTANTIVE. The "a brand-new prd.md/design.md is never logistics" fail-safe only checked status=="added" — a rename that turns a previously non-canonical file (e.g. notes.md) into prd.md/design.md is reported by GitHub as status=="renamed" and slipped through. Fixed by also forcing SUBSTANTIVE when a rename's new basename is canonical and its previous basename wasn't the same canonical name — same-basename renames (the real PR osac-project#174 shape, e.g. README.md -> README.md under a new directory) are unaffected. Full suite: 134 passed, 5 subtests passed (was 126 before this commit). Golden PRs unchanged: osac-project#174 -> LOGISTICS_ONLY; osac-project#168/osac-project#172/osac-project#173 -> SUBSTANTIVE. EP_REVIEW_SKIP_LOGISTICS remains default-off; no Jira fetching or PR title/body key fallback introduced. Deferred cleanup/simplification suggestions from the same review (duplicated comment-rendering helpers, duplicated canonical filename constants, main() decision-object refactor, generic semantic parser, dynamic frontmatter schema, type-hint/docstring cleanup) are intentionally not included — out of scope for this correctness-fix pass. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
… bypass The security pre-flight review on this branch flagged that the logistics-only classifier's "safe link/path substitution" category masked an entire markdown link (visible label included), so a PR could rewrite a link's human-readable label to arbitrary attacker-controlled text while the surrounding line stayed byte-identical, and still classify as a safe logistics-only change. Only tolerate a changing link label when it's a bare filename-shaped token (the real PR osac-project#174 rename shape, e.g. "old-name.md" -> "README.md") - arbitrary prose in a link label now makes the file fall through to SUBSTANTIVE, same as any other unrecognized change. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
…urity re-review The prior fix (5d2db8c) only partially closed the link-label masking gap: running the markdown-link pass and the bare-URL/path pass sequentially let a URL-shaped link label slip past the label protection on the second pass. Separately, the bare URL/path pattern had no real terminal boundary, so attacker-appended text could ride along immediately after a legitimate path with no separator (e.g. ".../design.md-and-ignore-all- safety-checks"). - Combine link and bare-URL/path matching into a single regex alternation evaluated in one left-to-right pass, so a link label's characters are never re-offered to the bare-URL/path branch afterwards. - Anchor the bare "/enhancements/..." path pattern to the terminator shapes actually observed in real EP content (quote, backtick, end of line, or a known doc extension) instead of an open-ended charset. - Require link labels tolerated as "safe to change" to have an actual file extension (the real PR osac-project#174 shape), not just be dash/alnum text — an ordinary single-word prose label is no longer mistaken for a filename. A directory-only bare path with none of those terminators is now simply not recognized by category 3 at all (fail-safe by omission), rather than attempting to bound an inherently ambiguous shape. EP_REVIEW_SKIP_LOGISTICS still defaults to false, so this closes a latent gap rather than an active one. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
Third security re-review pass found two CRITICAL gaps in the Phase B logistics-only classifier: a bare `https?://\S+` matcher with the same unbounded-suffix problem previously fixed for bare paths, and link-label changes accepted merely because they looked filename-shaped (e.g. an attacker-controlled label like "security-team-approved-merge-now.md"). Remove the bare-URL alternative entirely — no real golden fixture (osac-project#168/osac-project#172/osac-project#173/osac-project#174) ever needs it; every https:// in real EP content sits inside a markdown-link target or the allow-listed tracking-link frontmatter field. Replace shape-based label leniency with two-part provenance: a changed link label is only safe when (1) the old/new targets resolve to an exact (previous_filename, filename) pair GitHub reports as a real rename in this same PR, AND (2) the new label matches that file's new basename AND that basename is in a narrow allowlist of real EP document names (README.md/prd.md/design.md). Rename provenance alone isn't sufficient — the PR author controls the rename itself, so a legitimate GitHub rename to an arbitrary filename followed by a "correct" relabel would otherwise still pass. Verified against the real PR osac-project#174 fixture that its sole label change (03-prd.md -> README.md) is backed by an in-PR rename record; verified that markdown-link targets and bare "/enhancements/..." paths must stay provenance-free, since osac-project#174 itself contains cross-references to renames from earlier PRs with no corroborating record in this PR's own file list. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
…m list Security preflight found that the "does this introduce a first-seen reviewable EP identity" guard only special-cased status "added" and "renamed", leaving GitHub's documented "copied" status (which can introduce a first-seen reviewable path, e.g. via a copy with only a narrow-safe tracking-link patch) as an unguarded bypass. Replace the two ad hoc status checks with a structural invariant: _introduces_new_reviewable_identity() treats "modified" (and a same-basename "renamed", the PR osac-project#174 shape) as the only identity-preserving transitions, and forces SUBSTANTIVE for every other status — added, copied, changed, unchanged, removed, or any future status this enum grows — when the basename is reviewable. This closes the whole class in one pass instead of re-enumerating one status at a time each review round. Also import ep_paths.CANONICAL_FILENAMES directly instead of maintaining an independent literal copy, per the review's non-blocking drift-risk note — the two constants can no longer silently diverge. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
…the final field PR review (ItzikEzra-rh) found HIGH: the allow-listed-frontmatter shortcut in _hunk_is_safe decided from the change group's FINAL field state only, via _update_frontmatter_field's context-line-oriented leniency (it leaves the field unchanged for any line that isn't itself a recognized `key:` or the `---` delimiter, to support real YAML continuations like ` - value`). That same leniency let an unrelated, unindented prose line silently "inherit" whatever field was last tracked, so a contiguous +/- group (no separating context line) ending on an allow-listed last-updated/tracking-link line waved through arbitrary injected or deleted prose earlier in the same group. Track a sticky group_all_safe flag and verify every changed line individually: a continuation is only trusted when it's indented (real YAML list/wrapped-value shape) AND a field is already tracked; anything else (unindented, non-key, non-delimiter) breaks the chain to None, which can never be allow-listed. Sticky means a later line re-establishing a valid key can't retroactively excuse an earlier unsafe line in the same group. Legitimate multi-line YAML continuation behavior (PR osac-project#174's tracking-link list-item edits) is unaffected, since those continuation lines are indented and never break the chain. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
empovit
pushed a commit
to empovit/osac-enhancement-proposals
that referenced
this pull request
Aug 31, 2026
PR review (tchughesiv) noted a remaining nit: markdown-link targets were treated as "always safe to change" even when the label stayed identical, so a same-label retarget to an arbitrary external URL classified LOGISTICS_ONLY unconditionally. The target isn't rendered as visible text, but it's still where a reader ends up navigating to, so an external retarget is its own attacker-controlled-destination risk independent of the label check. Add _target_is_repo_local: whenever a markdown-link target actually changes, both the old and new target must be one of the shapes real PR osac-project#174 evidence needs — a bare "/enhancements/..." path, a GitHub blob URL for this exact repo, or a relative repo path with no URI scheme and no other absolute-path prefix. An unchanged target is never re-validated, so an already-external link elsewhere on the same line (e.g. osac-project#174's cross-repo applyFieldDefinitions() link) is unaffected. Rename-pair provenance for labels is unchanged; this is an additional, independent check specifically for the target's destination. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Omer Avi <omeravi23@gmail.com>
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
4 participants
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
Follow-up to Eran Cohen's comment on OSAC-2870, which re-examined the 8 directories that were still unrenamed after #139/#144/#149 merged and found resolvable Jira keys for 5 of them by searching Jira Features directly (rather than only checking each doc's own frontmatter/PR title).
Each of Eran's 4 matches was independently re-verified here — and one (
catalog-items) was corrected — using three signals per directory: content match, an explicit "Enhancement Proposal" link in the Jira issue's own description pointing back at the originating GitHub PR (where present), and date correlation (the directory's/PR's creation date vs. the Jira issue's creation date — these EPs predate Jira Feature tracking, so tickets were consistently backfilled 1–4 months later).Changes
catalog-itemsOSAC-1002-catalog-itemsOSAC-1531(a directory-name-similarity match).OSAC-1531's own PR (#129) already lives atenhancements/OSAC-1531-default-catalog-items/prd.md— a different, later, narrower feature ("auto-publish default catalog items at install time") that explicitly lists "Modifications to the fulfillment-service private API" as out of scope, i.e. it assumes this directory's catalog-item API already exists. The correct match isOSAC-1002("Support Catalog items for CaaS & VMaaS - Part 1") — its Jira description literally links.../pull/17, the exact PR that created this directory. No actual duplication with #129 exists; both directories are independently correct.dns-apiOSAC-1050-dns-api2026-03-17(same day).OSAC-1050's problem statement ("OSAC currently hardcodes AWS Route 53...") is nearly verbatim identical to the doc's own. Reporter Dan Manor = doc authordmanor.organizationsOSAC-1030-organizations2025-12-21(same day).OSAC-1030's description explicitly links.../pull/14.vm-api-fieldsOSAC-1034-vm-api-fields2026-01-27(same day).OSAC-1034's parent Epic (OSAC-61, same title) is authored by Michael Hrivnak — matches doc authormhrivnak.repository-consolidationOSAC-1732-repository-consolidation(Epic-level)2026-05-07;OSAC-1732was created2026-06-24, the same day PR #40 merged.OSAC-1732's description explicitly links.../pull/40. Self-authored and self-filed by Eran Cohen (ercohen@redhat.comin the doc). Used the Epic, not its parent FeatureOSAC-2053("CI Modernization & Quality") — that Feature is a broad umbrella over 10 unrelated sibling Epics (CI dashboards, Triagent, cluster-tool, etc.), whereasOSAC-1732's own title is a word-for-word match of this EP's title.All 5 renames used
git mvto preserve file history.catalog-items/andorganizations/each also contain aui-design.mdalongside theREADME.md, both moved together.Cross-references updated
Searched the full repo for references to all 5 old directory names, then re-ran the sweep against all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) as a validation pass — that second sweep caught 2 more that earlier PRs had missed. Final count: 16 reference-line edits across 11 files:
OSAC-1002-catalog-items/ui-design.mdsee-alsoself-reference to the pre-renamecatalog-itemspathOSAC-1030-organizations/ui-design.md.../blob/main/enhancements/organizations/README.md) pointing at its own sibling file's pre-rename pathOSAC-985-metering-and-usage-tracking/prd.mdcatalog-items,organizationsOSAC-985-metering-and-usage-tracking/design.mdcatalog-items,organizations,vm-instance-types(this last one was already stale before this PR — missed by #144, which renamedvm-instance-types→OSAC-46-vm-instance-typeswhilemetering-and-usage-trackingwas still deferred behind an open PR, so it fell outside that PR's cross-reference sweep)OSAC-1269-cluster-version-api/design.mdcatalog-itemsOSAC-979-image-management/README.mdcatalog-items(×2)OSAC-1118-baremetal-instance-api/README.mdcatalog-itemsOSAC-1421-cluster-and-vm-provisioning-wizard/prd.mdcatalog-itemsOSAC-1421-cluster-and-vm-provisioning-wizard/design.mdcatalog-itemsOSAC-1567-secret-management/design.mdorganizationsOSAC-1330-type-safe-resource-references/design.mdnetworking(also already stale before this PR —#121self-renamed this directory in its own branch, so it never went through ournetworking→OSAC-356-networkingcross-reference sweep from #144 either)The two bolded ones aren't new breakage from this PR — they're pre-existing staleness from earlier PRs that a full-repo audit (rather than a scoped one) caught. Fixed here since they were already found.
Frontmatter
Filled in
tracking-linkfor all 5 (previously empty placeholder orTBD) with the resolved Jirabrowse/link. LeftOSAC-1030-organizations/ui-design.md's ownJira: OSAC-2792field untouched — that's a distinct, more specific ticket for the UI-only slice of this feature, which is a normal and expected pattern (seecatalog-items/ui-design.md's separate PR-based tracking-link for the same reason).Notes for reviewers
OSAC-1002-catalog-items— @mhrivnakOSAC-1050-dns-api— @danmanorOSAC-1030-organizations— @avishaytOSAC-1034-vm-api-fields— @mhrivnakOSAC-1732-repository-consolidation— @eranco74 (also the Jira commenter who identified 4 of these 5)vmaas,bare-metal-fulfillment,tenant-specific-storageclasses— all confirmed to genuinely predate Jira Feature tracking with no resolvable key (tracking-link: None/TBDand no Jira Feature exists to link, per his analysis). Left as-is; not addressed by this PR.Testing
pre-commit run --all-fileslocally — all hooks pass, includingcheck-ep-naming, after both commits..md) that no stray references remain for any of the 5 old directory names, or for any of the other 36 retired directory names from the rest of the OSAC-2870 effort (excluding known test fixtures intest_check_ep_naming.py, which intentionally use retired names as illustrative path strings).fulfillment-service,osac-operator,osac-installer,osac-test-infra,osac-ui,osac-aap,docs) viagit grepfor cross-repo references to any of the 5 old paths — none found.git log --followon each renamed file traces cleanly back through its full history to the original PR that created it.main(0 commits behind).Summary by CodeRabbit