Skip to content

fix(supply-chain): restore simple-path cycle regressions after Bolt rewrite - #886

Closed
cursor[bot] wants to merge 14 commits into
developfrom
cursor/bc-ebb283b7-1211-4109-ba04-b28c47d94938-16d5
Closed

fix(supply-chain): restore simple-path cycle regressions after Bolt rewrite#886
cursor[bot] wants to merge 14 commits into
developfrom
cursor/bc-ebb283b7-1211-4109-ba04-b28c47d94938-16d5

Conversation

@cursor

@cursor cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown

Closed as proven duplicate of canonical #867

Exact duplicate/supersession evidence was revalidated immediately before closure:

No merge evidence from this PR is transferred. #867 must obtain fresh exact-head CI/security/SBOM/coverage/review evidence on f270d3df…; inherited #783 and central dependency gates remain unchanged.

seonghobae and others added 14 commits August 16, 2026 14:07
Bolt reapplied a shared (package_key, matched_count) cache on top of the
already-restored simple-path walk. That cache lets one package key satisfy
two owner-chain positions through a cycle. Restore path-local frozenset
prevention, keep the cycle and distinct-key regressions in two test
modules, drop the unauthorized pdfjs-dist Trivy ignore, and record the
unsafe optimization as a rejected change.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
…ewrite

Head bcf22ee replaced the validated frozenset walk with recursive DFS and
deleted the cycle/distinct-key tests plus the simple-path policy citation.
Restore the locked iterative walk, both regression modules, and the Cormen
et al. (2022) owner-chain authority so the next rewrite cannot drop the lock.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
@cursor
cursor Bot requested a review from seonghobae August 16, 2026 16:30
@seonghobae
seonghobae marked this pull request as ready for review August 16, 2026 16:31
@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Author

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Prefer #887 over this restore

This head f270d3df correctly restores the dbcbe6d9 frozenset walk, cycle/distinct-key tests, and Cormen et al. (2022) citation. Keep it closed or unmerged once #887 lands.

#887 593b6fe6 is the same restore plus test_dependency_policy_documents_named_dependency_path_simple_path_authority, which fails on the Bolt policy file and would have blocked the bcf22eea deletion. Do not open a fourth restore PR. Do not merge #867 at bcf22eea.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

@seonghobae seonghobae closed this Aug 16, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of f270d3df vs develop acdbea63, with the full cargo_lock_has_named_dependency_path body (not just the docstring hunk) and a file-level compare to draft #887 593b6fe6.

CodeRabbit CLI is not installed in this environment (coderabbit: command not found); this assessment is from the required walk/diff/pytest checks.

Strengths

  • The walk at scripts/checks/verify_supply_chain.py:1998-2012 is an iterative path-local frozenset stack (pending.pop(), next_seen = seen | {current}). It is not recursive DFS and it does not use a shared (package_key, matched_count) cache. Develop already had this body; this PR only documents the simple-path rule.
  • Cycle fixture root → alpha@1 → beta → alpha@1 with query ("alpha", "alpha", "charlie") is False. Distinct keys alpha@1 then alpha@2 are True. Direct ("alpha", "charlie") on the cyclic graph is True (reproduced in the PR worktree; the last one is not locked).
  • The four pytest cases are real load_module calls of the matcher, not mocks. All four passed.
  • Policy text cites Cormen et al. (2022, Appendix B.4) in APA 7th, and .jules/bolt.md rejects the shared-cache rewrite. No .trivyignore / pdfjs-dist / CVE-2026-16633 change.

Issues

Critical

None.

Important

  1. Missing required lock for direct ("alpha", "charlie") on the cyclic graphservices/analysis-engine/tests/test_supply_chain_dependency_path_cycles.py:19-23 and the twin at test_supply_chain_policy.py:5126-5130 only lock the False cycle case. A rewrite that refuses every remaining-name walk once a back-edge exists would still pass both locked tests and break owner-chain matching. The PR description says this case was reproduced; it is not a regression test.
  2. This head is not the best landing vehicle. #886 and #867 now share SHA f270d3df. Draft #887 593b6fe6 is the same restore plus test_dependency_policy_documents_named_dependency_path_simple_path_authority, which is RED on develop's policy file and GREEN on this policy text. That is the lock that would have caught the Bolt deletion of the citation/section. After reading both diffs, prefer #887; do not treat it as better on the walk (the walk files are identical).

Minor

  • Cycle and distinct-key cases are duplicated across test_supply_chain_dependency_path_cycles.py and test_supply_chain_policy.py with no extra assertion in the policy module.
  • .jules/bolt.md:64 appends the new learning without a blank line after the previous entry.

Recommendations

  • Land #887 (mark ready). Close #886 and #867 as the same-SHA restore without the policy-text lock. Do not open a fourth restore PR. Do not merge #867 at any older DFS head.
  • On the landing head, add assert cargo_lock_has_named_dependency_path(..., ("alpha", "charlie")) on the existing cycle fixture.
  • Keep inherited pdfjs-dist / CVE-2026-16633 on #783.

Assessment

Ready to merge? With fixes. The restore is behaviorally correct, but do not merge #886 over #887, and do not treat the lock set as complete until the direct cyclic-graph case is a real test.

package_dependencies,
"root 1.0.0",
("alpha", "alpha", "charlie"),
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This locks the False cycle case only. The required counterpart is still missing: on this same fixture, ("alpha", "charlie") must stay True (direct remaining-name match must not die just because beta points back at alpha@1). I reproduced True on this head; please add that assert here (and in the policy-module twin) on the landing vehicle. A walk that skips every other edge after seeing a back-edge would still pass this test and break owner-chain matching.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer #887; do not merge this restore over the policy-text lock

Exact head f270d3df restores the dbcbe6d9 authority: iterative path-local frozenset in cargo_lock_has_named_dependency_path, cycle/distinct-key regressions in both test modules, Cormen et al. (2022, Appendix B.4) APA citation, and the rejected shared-cache note in .jules/bolt.md. The walk is not recursive DFS and does not use a shared (package_key, matched_count) cache. Reproduced on this head: cycle False, distinct keys True, direct ("alpha", "charlie") True. No .trivyignore / pdfjs-dist / CVE-2026-16633 change.

#867 now points at the same SHA. Draft #887 593b6fe6 is that restore plus test_dependency_policy_documents_named_dependency_path_simple_path_authority, which fails on the Bolt-deleted policy file and would have blocked the bcf22eea citation deletion. Land #887. Keep this PR and #867 unmerged once #887 is ready. Do not open another restore PR. Do not merge any older DFS head.

Both this head and #887 still omit a locked assert for direct ("alpha", "charlie") on the cyclic fixture. Add that on the landing head so a rewrite that refuses every remaining-name walk after a back-edge cannot stay green.

This run cannot approve or merge. Independent review remains with seonghobae. Inherited JS vulns stay on #783. Next buyer-visible gap is issue #770; do not fold it here.

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

Comment on lines +19 to +23
assert not supply_chain.cargo_lock_has_named_dependency_path(
package_dependencies,
"root 1.0.0",
("alpha", "alpha", "charlie"),
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This locks the False cycle case only. On the same fixture, also assert cargo_lock_has_named_dependency_path(..., ("alpha", "charlie")) is True. A rewrite that treats any back-edge as "no remaining-name match" would still pass this test and break a real owner-chain. Add that assert on the #887 landing head, not in a fourth restore PR.

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.

2 participants