Skip to content

refactor(cli): drop comments restating adjacent dns/subdomain code#330

Open
TBX3D wants to merge 1 commit into
vmfunc:mainfrom
TBX3D:feat/per-target-accumulator
Open

refactor(cli): drop comments restating adjacent dns/subdomain code#330
TBX3D wants to merge 1 commit into
vmfunc:mainfrom
TBX3D:feat/per-target-accumulator

Conversation

@TBX3D

@TBX3D TBX3D commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

two comments in scanTarget restate the line they sit above: dnsResults = result // Store the DNS results, and a // Only run subdomain takeover check if DNS scan is enabled above the if app.settings.SubdomainTakeover it describes. drop both.

this PR previously carried the scanTarget/targetScan extraction as well. that landed on main in #367 with the same signature and the same sif_scantarget_test.go, so only the comment trim is left here.

@TBX3D
TBX3D requested a review from vmfunc as a code owner July 4, 2026 01:38
@github-actions github-actions Bot added size/xl 500+ lines changed tests test changes and removed size/xl 500+ lines changed labels Jul 4, 2026
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

pr summary

1 files changed (+1 -2)

category files
go source 1

@codecov-commenter

codecov-commenter commented Jul 4, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@a38ba0a). Learn more about missing BASE report.

Files with missing lines Patch % Lines
sif.go 0.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #330   +/-   ##
=======================================
  Coverage        ?   64.87%           
=======================================
  Files           ?       88           
  Lines           ?     7858           
  Branches        ?        0           
=======================================
  Hits            ?     5098           
  Misses          ?     2367           
  Partials        ?      393           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TBX3D
TBX3D force-pushed the feat/per-target-accumulator branch from 73221e0 to d5e235c Compare July 4, 2026 02:35
@github-actions github-actions Bot added the size/xl 500+ lines changed label Jul 4, 2026

@vmfunc vmfunc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

clean extraction. the only spot that isn't mechanical is the apimode marshal-failure path, and you got it right: return targetScan{scansRun: scansRun, logFiles: logFiles}, nil keeps the scan labels and log files but drops findings/report, same as the old continue. logFiles moving to a local and merging back in Run still lands in app.logFiles before PrintSummary, so the summary is unchanged, and nothing reads app.logFiles mid-scan so the timing shift is invisible.

TestScanTargetIsolatesPerTargetState is the right test for the seam. two sequential targets each reporting a single scan is exactly what a shared slice would break.

conflicts with main though, rebase and it's in.

@TBX3D
TBX3D force-pushed the feat/per-target-accumulator branch from 8b0c53d to a350757 Compare July 23, 2026 00:10
@TBX3D TBX3D changed the title refactor(cli): extract per-target scan into scanTarget refactor(cli): drop comments restating adjacent dns/subdomain code Jul 23, 2026
@TBX3D

TBX3D commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

rebased, and there is almost nothing left. #367 landed scanTarget/targetScan with the same signature and the same sif_scantarget_test.go, so the extraction this PR proposed is upstream already. only the two-line comment trim survives; retitled and rewrote the description to match.

that means your approval no longer covers what is here, so please re-read before merging, or close it if a two-comment cleanup is not worth the churn. either is fine by me.

@github-actions github-actions Bot added size/xs <10 lines changed and removed size/xl 500+ lines changed labels Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xs <10 lines changed tests test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants