Skip to content

chore(seo): trim the IndexNow comments to what the code needs - #420

Merged
jfrench9 merged 1 commit into
mainfrom
chore/trim-indexnow-comment
Sep 21, 2026
Merged

jfrench9 merged 1 commit into
mainfrom
chore/trim-indexnow-comment

Conversation

@jfrench9

Copy link
Copy Markdown
Member

17 lines of comment over a 7-line block in the Dockerfile, and a 10-line header on the workflow. The rationale they carried — blast radius, the ECR metadata bound, the engine list — is already in #418/#419 and the vault.

Kept: why the key comes from a build arg, and why the trigger is a schedule. Both are things a reader can't infer from the code.

No behaviour change. npm run test:all green.

🤖 Generated with Claude Code

The rationale they carried — blast radius, the ECR metadata bound, the
engine list — is in the PRs and the vault. What a reader here needs is why
the key comes from a build arg and why the trigger is a schedule.
@github-actions github-actions Bot added the change:normal Normal change: automated review documented on the pull request before merge label Sep 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 21, 2026

Copy link
Copy Markdown

Claude finished @jfrench9's task in 1m 15s —— View job


Reviewing PR #420

  • Gather context (diff, files)
  • Review Dockerfile comment trim
  • Review workflow comment trim
  • Post review feedback

Review

Comment-only diff, confirmed — no RUN/ARG/cron/permissions/with: values changed in either file, only prose. Both trims read cleanly and the two rationales the PR body calls out as "kept" are in fact still there:

  • .github/workflows/indexnow.yml:6-8 — the schedule-vs-deploy-hook reasoning survives, just tighter.
  • Dockerfile:107-110 — the build-arg-vs-committed-file reasoning survives too.

One thing worth a second look before merging: Dockerfile:107-110 drops the line that used to read "The value is recorded in this image's build metadata, so it is readable by anyone who can pull from ECR... rotating it is a new arg plus a new variable." That's the one part of the trimmed prose that isn't just restating what the code already shows — it's the answer to "wait, isn't putting a value in an ARG normally a Docker anti-pattern for anything secret-ish?", which a future reader (or a security-linting pass) could plausibly re-raise. The PR body's rationale is that this is already captured in #418/#419 and "the vault," which is a reasonable place to keep it, but neither is discoverable from the Dockerfile itself. Not a blocker — just flagging the tradeoff (inline discoverability vs. duplicated rationale) so it's a conscious call rather than an oversight. Fix this →

Everything else:

  • .github/workflows/indexnow.yml:12 — dropping "every three hours" from the cron comment is fine; the cadence is directly legible from '17 */3 * * *' right below it, so keeping it would've been redundant.
  • .github/workflows/indexnow.yml:31 — collapsing the actions: write comment to an inline trailing comment is a reasonable compression, still explains the "why."
  • .github/workflows/indexnow.yml:37-38 — the "public once served, so a variable not a secret" framing is preserved accurately.
  • npm run test:all: neither changed file is exercised by lint/typecheck/test/cfn-lint (no JS/TS touched, and cf-lint targets CloudFormation, not workflow YAML), so a green run here is expected and doesn't really validate the change — the actual check is just reading the comments, which line up with the current code.

No behavior change, nothing to fix functionally. The metadata-exposure line is the only content call worth the author's confirmation before merge.

@jfrench9
jfrench9 merged commit 5e9c36b into main Sep 21, 2026
7 checks passed
@jfrench9
jfrench9 deleted the chore/trim-indexnow-comment branch September 21, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:normal Normal change: automated review documented on the pull request before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant