Skip to content

docs(solutions): capture diagnostic-discipline + Issues-API race patterns#3350

Merged
marcusrbrown merged 1 commit into
mainfrom
docs/compound-2026-05-20-diagnostic-loop-and-issues-race
May 22, 2026
Merged

docs(solutions): capture diagnostic-discipline + Issues-API race patterns#3350
marcusrbrown merged 1 commit into
mainfrom
docs/compound-2026-05-20-diagnostic-loop-and-issues-race

Conversation

@marcusrbrown
Copy link
Copy Markdown
Collaborator

Two best-practices learnings from today's session captured as durable docs/solutions/ entries.

diagnostic-patches-observability-discipline-2026-05-20.md

The 3-PR investigation (#3344 \u2192 #3346 \u2192 #3347) where each patch revealed the next layer of a swallowed-error problem in the Survey Repo privacy gate. Four rules with verbatim code examples:

  1. Never 2>/dev/null a workflow gate's failure-path command \u2014 capture stderr to a tempfile and cat it on failure between marker lines
  2. Beware leading dashes in printf format strings \u2014 bash builtin rejects ---... as an option, use printf '%s\n' '--- text ---' instead
  3. Cross-org public reads use App tokens, not user PATs \u2014 fine-grained PATs hit per-org lifetime policies, App tokens auto-rotate and sidestep that class of footguns entirely
  4. Keep diagnostic patches bounded \u2014 fix the immediate blocker, not every symmetric instance; that's a separate cleanup PR

Cross-links survey-workflow-side-privacy-gate-2026-05-16.md (companion defense-in-depth doc on the same workflow), jq-falsy-coalesce-trap-in-shell-gates-2026-05-17.md, autonomous-pipeline-silent-failures-2026-04-19.md, and the three PRs plus follow-up issues #3345/#3349.

github-issues-api-same-run-eventual-consistency-2026-05-20.md

The in-memory created-ID Set<string> pattern that closes the race where issues.listForRepo lags behind issues.create within the same process (PR #3321, scripts/reconcile-repos.ts). The rule: any code path that BOTH creates a GitHub issue AND later lists issues to decide on creation within the same process must carry an in-memory Set<string> of created identifiers through subsequent stages \u2014 the Set is the source of truth for the lifetime of the run, NOT issues.listForRepo.

Cross-links merge-data-pr-github-422-race-recovery-2026-05-02.md as the closest analogue (same family on the Pulls API, different fix shape), bootstrap-data-branch-before-autonomous-writes-2026-05-09.md, PR #3321, source issues #3307/#3308, and follow-ups #3319/#3332.

Why now

Both topics were freshly cached from today's work. Capturing them while the investigation context is hot prevents the typical degradation where compound docs land days later with vague reconstructions. Smart note #115 (which had been tracking the Issues-API race writeup since PR #3321 merged) is dismissed by this PR.

…erns

Two best-practices learnings from today's session:

1. diagnostic-patches-observability-discipline — the 3-PR investigation
   (PR #3344#3346#3347) where swallowed stderr blocked progress,
   the diagnostic patch itself had a bash builtin printf bug, and the
   real fix was an App-token swap for cross-org public reads. Four rules:
   - never `2>/dev/null` a workflow gate's failure-path
   - beware leading dashes in `printf` format strings on bash builtin
   - cross-org public reads use App tokens, not user PATs
   - keep diagnostic patches bounded (one blocker at a time)

2. github-issues-api-same-run-eventual-consistency — the in-memory
   created-ID `Set<string>` pattern that closes the race where
   `issues.listForRepo` lags behind `issues.create` within the same
   process (PR #3321). Pairs with the existing 422-race doc on the
   Pulls API as the same eventual-consistency family.

Both are knowledge-track, both link to closest analogues, both list
the relevant PRs and follow-up issues.
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Docs-only PR adding two docs/solutions/best-practices/ entries. Both files are grounded in real, traceable PRs (#3344/#3346/#3347 for the diagnostic-discipline doc, #3321 for the Issues-API race doc), cross-references resolve to existing files in security-issues/, workflow-issues/, runtime-errors/, and integration-issues/, and all referenced issues (#3307, #3308, #3319, #3332, #3345, #3349) exist with the states the docs imply.

The technical content is sharp:

  • The bash-builtin-vs-coreutils printf portability gap is a real Ubuntu-runner-only footgun that's invisible on macOS — exactly the kind of thing that belongs in durable docs.
  • The in-memory Set<string> dedup pattern is the structurally correct answer to GitHub's asymmetric write→read consistency window, and the regression-test shape (mock listForRepo returning [], pass the just-created ID through the set, assert no second create) is the right way to lock it in.
  • The "App tokens over fine-grained PATs for cross-org public reads" guidance correctly flags the per-org lifetime policy as the structural reason, not just an incidental error.

Blocking issues

None.

Non-blocking concerns

  • Frontmatter verified value drift. Both new files use verified: 2026-05-20 (date). Peers in the same directory (autonomous-pipeline-minimum-progress-floor-2026-05-17.md, loose-then-tight-schema-migration-pattern-2026-05-05.md) use verified: true. Pick one convention for the directory; if dates are the new shape, fine, but worth a follow-up to normalize the older entries.
  • Self-acknowledged staleness in a cross-link. The diagnostic-discipline doc notes that survey-workflow-side-privacy-gate-2026-05-16.md still shows the old 2>/dev/null + leading-dash printf patterns. Filing a follow-up to update that doc (or adding a stale-banner) would prevent the next reader from copy-pasting the obsolete shape.
  • Path normalization for cross-links. Links use ../security-issues/..., ../workflow-issues/..., etc. Verified all targets exist on disk — no action needed, just confirming the relative paths resolve.

Missing tests

N/A — documentation-only change with no executable surface. The Issues-API doc itself prescribes the regression-test shape that should exist in the codebase (per PR #3321); confirming that test landed with #3321 is out of scope for this review.

Risk assessment

LOW. Additive markdown under docs/solutions/. No workflow, action, dependency, or runtime code touched. Worst-case failure mode is a reader following stale guidance from a cross-linked doc — already called out in the text. Approving without inline comments because every observation is meta-organizational rather than file-specific.


Run Summary
Field Value
Event pull_request
Repository fro-bot/.github
Run ID 26186319751
Cache hit
Session ses_1b90bedb1ffeYOtFxYBL1ZfIII

@marcusrbrown marcusrbrown merged commit 0470d6a into main May 22, 2026
12 checks passed
@marcusrbrown marcusrbrown deleted the docs/compound-2026-05-20-diagnostic-loop-and-issues-race branch May 22, 2026 18:05
marcusrbrown added a commit that referenced this pull request May 23, 2026
…h privacy-gate code (#3367)

* docs(solutions): standardize verified field on ISO dates and refresh privacy-gate code

Two non-blocking concerns from PR #3350 Fro Bot review (issue #3351):

1. Convention sweep: 13 docs/solutions/ entries had verified: true; converted
   to ISO date (verified: YYYY-MM-DD) matching the file's date suffix. Two
   recent entries already used the date shape; the directory is now consistent.

2. Stale-code refresh in survey-workflow-side-privacy-gate-2026-05-16.md:
   - Resolve step now shows App token mint + tempfile stderr capture (PRs
     #3344, #3346, #3347) instead of pre-fix `FRO_BOT_PAT` + `2>/dev/null`.
   - Recheck step honestly notes the asymmetry: still uses `2>/dev/null`
     and is tracked as follow-up #3345.
   - Added prevention rule #1 (App tokens over fine-grained PATs for
     cross-org public reads) and #10 (capture `gh api` stderr to tempfile).
   - Cross-links the diagnostic-discipline compound doc and the three PRs
     that produced the current production pattern.
   - last_updated bumped to 2026-05-23.

* docs(solutions): show recheck snippet verbatim to make asymmetry visible

PR #3367 NBC #1: the recheck snippet was elided as a prose comment, which
hid the actual 2>/dev/null pattern from readers comparing it to the
adjacent (refreshed) resolve step. Now shows the recheck body verbatim
with an inline NOTE flagging the asymmetry. The follow-up reference to
#3345 remains in the prose paragraph below.
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