Skip to content

fix(warehouse): normalize --paths input before tracked-set membership check (PER-184)#151

Merged
Shadowsong27 merged 2 commits into
mainfrom
feat/per-184-normalize-paths
May 20, 2026
Merged

fix(warehouse): normalize --paths input before tracked-set membership check (PER-184)#151
Shadowsong27 merged 2 commits into
mainfrom
feat/per-184-normalize-paths

Conversation

@Shadowsong27
Copy link
Copy Markdown
Owner

Summary

  • abc warehouse contribute --paths rejected ./prefix and //-containing path spellings even when they pointed at tracked artifacts.
  • Add _normalize_path() helper that strips ./, collapses //, rejects absolute paths and .. traversal, and returns POSIX-style strings.
  • Apply normalization to every --paths entry before tracked-set membership and use the normalized form as commit-paths.
  • 4 new test cases under TestContributePaths: leading ./, double //, absolute rejection, .. rejection.

Closes PER-184.

Test plan

  • pytest libs/beacon/tests/unit/warehouse/ libs/beacon/tests/unit/test_architecture.py → 94 passed
  • ruff check on touched files → clean
  • CI green
  • opencode-review bot clean

🤖 Generated by diligent-supervisor / delegate-to-oc (kimi-for-coding/k2p6)

@github-actions
Copy link
Copy Markdown
Contributor

OpenCode Review

Model: openai/gpt-5.5
Reviewed commit: 4cb113f874dac80f1ed516e61288b515994a07ae
Workflow run: https://github.com/Shadowsong27/agentic-beacon/actions/runs/26091187353

Findings

No findings.

Open Questions

None.

Notes

  • Review limited to the attached diff. The added normalization stays within the warehouse domain, and tests cover the primary changed path behaviors.

Token Usage

Input tokens: 10261
Output tokens: 47
Reasoning tokens: 516
Cache read tokens: 2560
Cache write tokens: 0
Total tokens: 13384

… check (PER-184)

`abc warehouse contribute --paths` was rejecting paths with `./` prefix or
redundant `//` separators because the membership check was a direct string
comparison against the tracked set.

Add `_normalize_path()` helper that:
- Strips `./` and collapses `//` via `os.path.normpath`.
- Rejects absolute paths with a clear error.
- Rejects parent-directory (`..`) traversal with a clear error.
- Returns POSIX-style forward slashes for cross-platform consistency.

Apply normalization to every `--paths` entry before checking membership
against `tracked_set`, and use the normalized form as the commit-paths
list so git sees the same canonical strings.

Test suite extended with 4 new cases under `TestContributePaths`:
leading `./`, double `//`, absolute rejection, and `..` rejection.

Closes PER-184.
@Shadowsong27 Shadowsong27 force-pushed the feat/per-184-normalize-paths branch from 4cb113f to 9b94588 Compare May 19, 2026 10:52
@github-actions
Copy link
Copy Markdown
Contributor

OpenCode Review

Model: openai/gpt-5.5
Reviewed commit: 9b9458847f490d6d5502c60a657a8ac90df98e30
Workflow run: https://github.com/Shadowsong27/agentic-beacon/actions/runs/26092553756

I’ll check the repository-specific context rules, then review only the supplied diff against those constraints.

Findings

No findings.

Open Questions

None.

Notes

  • The attached diff stays within the warehouse domain, adds focused unit coverage, and does not affect CLI layering or artifact adoption behavior.
  • Residual edge case: backslash-separated paths are not normalized on POSIX, but this does not appear to create a traversal risk on supported platforms.

Token Usage

Input tokens: 8113
Output tokens: 211
Reasoning tokens: 568
Cache read tokens: 30720
Cache write tokens: 0
Total tokens: 39612

@Shadowsong27
Copy link
Copy Markdown
Owner Author

Supervisor triage — bot round 1 + 2

Bot reviewed commits: 4cb113f (initial) and 9b945884 (post-rebase on 048b36e hash-refresh main).

Findings

  • None reported by either round.

Notes / residual risks (bot's "Notes" section)

  • "Residual edge case: backslash-separated paths are not normalized on POSIX, but this does not appear to create a traversal risk on supported platforms."
    • Decision: acknowledged, no action — beacon is macOS/Linux only (per AGENTS.md: "Platform: macOS / Linux only. Windows is rejected."). Backslash inputs on POSIX are valid filename characters; not a separator, not a security concern.

PR is READY for human review/merge. CI green across pre-commit, Unit Tests 3.12/3.13, E2E, and review jobs.

🤖 diligent-supervisor

Per PR #151 review: `_normalize_path` is a stateless helper with no
warehouse-domain knowledge — it belongs in utils/, not buried as a
private domain helper.

- New `beacon/utils/paths.py` with public `normalize_relative_path()`.
- `contribute.py` imports it; drops the now-unused `import os`.
- Add direct unit coverage in `tests/unit/utils/test_paths.py`
  (the contribute tests already cover it behaviorally).

No behaviour change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

OpenCode Review

Model: openai/gpt-5.5
Reviewed commit: b8d7288ce0b9a7c1bcdac9fd6a3a02815d6ce7c5
Workflow run: https://github.com/Shadowsong27/agentic-beacon/actions/runs/26142786927

Reviewing the attached diff against the repository guidance and Beacon-specific checks.

Findings

  • [Medium] Domain-specific helper added to utils - libs/beacon/src/beacon/utils/paths.py:1
    normalize_relative_path is only used by the warehouse contribute flow in this diff, so placing it in beacon.utils violates the project guidance that single-domain logic belongs in the owning domain. Move it under beacon.domains.warehouse, likely as a private helper/module, unless another domain consumer is added.

Open Questions

None.

Notes

  • Review was limited to the attached diff.

Token Usage

Input tokens: 8094
Output tokens: 194
Reasoning tokens: 542
Cache read tokens: 18944
Cache write tokens: 0
Total tokens: 27774

@Shadowsong27
Copy link
Copy Markdown
Owner Author

Supervisor triage — bot round 3 (post-refactor)

Bot reviewed commit: b8d7288 (the _normalize_pathutils/paths.py refactor). CI green across all jobs.

Findings

  • Medium"Domain-specific helper added to utilsnormalize_relative_path is only used by one domain, project guidance says single-domain logic belongs in the owning domain."
    • Decision: dismissed (won't fix) — repo-owner review decision.
    • AGENTS.md contains two criteria that genuinely conflict for this case:
      1. "utils/ — stateless helpers with no domain knowledge"normalize_relative_path is pure path-string manipulation, zero warehouse knowledge. Fits exactly.
      2. "If a module is only consumed by one domain, it belongs inside that domain" — true today (one consumer: warehouse contribute).
    • The move into utils/ was made on explicit PR-review feedback from the repo owner. The "no domain knowledge" criterion is the deciding one: this helper has none, and utils/ is its natural home (also the obvious landing spot for future path helpers — PER-184's ticket itself noted the same normalization need could recur). The single-consumer heuristic is a default, not an absolute, and is overridden here by the owner's call.

PR remains READY. No further changes.

🤖 diligent-supervisor

@Shadowsong27 Shadowsong27 merged commit daac8e0 into main May 20, 2026
9 checks passed
@Shadowsong27 Shadowsong27 deleted the feat/per-184-normalize-paths branch May 20, 2026 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant