Skip to content

fix(review): make review.memory a full config-as-code substitute - #4123

Closed
JSONbored wants to merge 1 commit into
mainfrom
fix/4101-config-as-code-kill-switch
Closed

fix(review): make review.memory a full config-as-code substitute#4123
JSONbored wants to merge 1 commit into
mainfrom
fix/4101-config-as-code-kill-switch

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • shouldApplyReviewMemory (src/review/review-memory-wire.ts) required BOTH GITTENSORY_REVIEW_MEMORY AND an explicit review.memory: true manifest opt-in (isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled) — a repo could only ever narrow what the operator already turned on, never fully control the feature through config.
  • Rewired it to the inlineComments/fixHandoff shape shipped in feat(review): upgrade inlineComments + fixHandoff to full config-as-code substitutes #4116 (Upgrade inlineComments + fixHandoff from required-AND opt-in to full config-as-code substitutes #4099): the operator's GITTENSORY_REVIEW_MEMORY flag remains an absolute master kill-switch (off ⇒ always false, regardless of the manifest), and an explicit per-repo .gittensory.yml review.memory: true/false now fully decides the outcome by itself.
  • review.memory has never had a GITTENSORY_REVIEW_REPOS cutover allowlist (confirmed: not in CONVERGED_FEATURE_KEYS, and env.d.ts's doc comment for GITTENSORY_REVIEW_MEMORY never mentioned one), so an unset manifest toggle stays false — byte-identical to the feature's original always-off default. resolveReviewMemoryManifestToggle (src/signals/focus-manifest.ts) already resolved the manifest correctly and needed no change.
  • Both call sites in src/queue/processors.ts (the auto-review pass and the @gittensory resolve command handler) compile unchanged against the new manifestToggle: boolean | undefined parameter type since they already pass resolveReviewMemoryManifestToggle(...)'s resolved boolean through.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Ran the full local gate: npm run test:ci (green — actionlint, migrations/schema-drift/env-reference/observability checks, cf-typegen, typecheck, unsharded test:coverage, test:workers, MCP/miner build+pack, rees:test, ui:openapi, docs:drift-check, command-reference:check, ui:lint/typecheck/test/build) plus npm audit --audit-level=moderate (0 vulnerabilities). Confirmed via coverage/lcov.info that src/review/review-memory-wire.ts is 21/21 lines and 22/22 branches covered (LF:21 LH:21, BRF:22 BRH:22), and the only hunk in src/queue/processors.ts is a doc-comment update (no coverable lines). Rebased onto origin/main after the local gate passed; re-ran typecheck plus the targeted review-memory-wire/review-fix-handoff/worker-entry-boundary/queue.test.ts (review-memory filter) suites post-rebase — all green.

If any required check was skipped, explain why:

  • None skipped.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/cookie/CORS/session surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed; ui:openapi:check confirms no drift.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changed.)
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. (N/A — backend-only change, no visible UI.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A — no changelog edit.)

UI Evidence

N/A — backend-only change (src/review/review-memory-wire.ts, src/queue/processors.ts comment, tests). No visible UI/frontend/docs surface.

Notes

shouldApplyReviewMemory previously required GITTENSORY_REVIEW_MEMORY AND
the per-repo manifest toggle (a repo could only narrow, never replace,
the env var). Match the inlineComments/fixHandoff shape (#4099): the env
flag is now an absolute kill-switch, and an explicit review.memory
true/false fully controls the feature by itself. review.memory has never
had a GITTENSORY_REVIEW_REPOS cutover allowlist, so an unset manifest
toggle stays byte-identical to the original always-off default.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.65%. Comparing base (1ec9f94) to head (870fe09).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4123   +/-   ##
=======================================
  Coverage   93.65%   93.65%           
=======================================
  Files         384      384           
  Lines       35865    35866    +1     
  Branches    13160    13160           
=======================================
+ Hits        33589    33590    +1     
  Misses       1618     1618           
  Partials      658      658           
Files with missing lines Coverage Δ
src/queue/processors.ts 95.15% <ø> (ø)
src/review/review-memory-wire.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 8, 2026
@loopover-orb

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-08 03:15:36 UTC

3 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • AI reviewers agree on a likely critical defect: src/review/review-memory-wire.ts: the rewritten `shouldApplyReviewMemory` is behaviorally identical to the old `isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled` for every value `resolveReviewMemoryManifestToggle` actually returns at its two call sites in src/queue/processors.ts, so despite the PR title/description claiming this makes review.memory 'fully' config-as-code-controlled, no runtime gating behavior actually changes — the operator env flag is still required to be true before the manifest has any effect, which is exactly the old 'narrow only' behavior being described as fixed. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This PR retypes `shouldApplyReviewMemory`'s second parameter from `boolean` to `boolean | undefined` and rewrites the surrounding comments/tests, but the actual gate logic is truth-table-identical to before: old code was `isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled`, new code is `if (!isReviewMemoryEnabled(env)) return false; return manifestToggle === true`. For every value the manifest resolver can produce at the only two call sites (both unchanged in this diff — `shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(...))`), both versions yield the same result: operator flag off → always false; operator flag on → result equals the manifest toggle (undefined/false collapse to false in both old and new). The PR title and description claim this closes the #4101 gap ('repo could only narrow, never fully control the feature'), but under AND logic the manifest already fully decided the outcome whenever the operator flag was on — there was no widening gap to close. This looks like a type-signature/comment-only change mislabeled as a behavioral config-as-code fix.

Blockers

  • src/review/review-memory-wire.ts: the rewritten `shouldApplyReviewMemory` is behaviorally identical to the old `isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled` for every value `resolveReviewMemoryManifestToggle` actually returns at its two call sites in src/queue/processors.ts, so despite the PR title/description claiming this makes review.memory 'fully' config-as-code-controlled, no runtime gating behavior actually changes — the operator env flag is still required to be true before the manifest has any effect, which is exactly the old 'narrow only' behavior being described as fixed.
Nits — 6 non-blocking
  • The new JSDoc/comment block in src/review/review-memory-wire.ts:2-30 is very long relative to the code it documents and reasserts the same 'no allowlist exists for this feature' claim three times — consider trimming to the operative precedence rule plus a link to Investigate + close the config-as-code gap for reviewMemory #4101.
  • test/unit/review-memory-wire.test.ts renames tests to reference Investigate + close the config-as-code gap for reviewMemory #4101 as a 'REGRESSION' fix, but since the truth table is unchanged from the prior test suite (just re-expressed with `undefined` instead of `false`), consider clarifying in the test description that this locks in existing behavior rather than fixing a regression.
  • External brief flags 'master kill-switch' phrasing (review-memory-wire.ts:2,22 and the test file) as non-inclusive terminology worth swapping for 'primary/main kill-switch' — low priority but easy to pick up while already editing these lines.
  • If `resolveReviewMemoryManifestToggle` genuinely can return `undefined` (which is presumably why the param type was widened), it'd be worth a one-line note in the comment confirming that today it never does at these two call sites, so a future caller doesn't assume `undefined` implies some different semantic than `false`.
  • Before merging, confirm with a concrete call trace whether resolveReviewMemoryManifestToggle (src/signals/focus-manifest.ts) can return undefined at either processors.ts call site today; if it cannot, this PR should be scoped as 'widen the type for future-proofing' rather than 'close a config-as-code gap', since no gap exists in the current AND-based gating.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Concerns raised — review before merging

  • src/review/review-memory-wire.ts: the rewritten `shouldApplyReviewMemory` is behaviorally identical to the old `isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled` for every value `resolveReviewMemoryManifestToggle` actually returns at its two call sites in src/queue/processors.ts, so despite the PR title/description claiming this makes review.memory 'fully' config-as-code-controlled, no runtime gating behavior actually changes — the operator env flag is still required to be true before the manifest has any effect, which is exactly the old 'narrow only' behavior being described as fixed.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #4101
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 509 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 509 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Linked issue satisfaction

Partially addressed
The PR replicates the intended behavior (env kill-switch as master, per-repo review.memory override fully deciding when set, unset preserving prior always-off default) and adds matching regression/unit tests, satisfying the issue's expected outcome. However the issue explicitly asks to migrate the gate onto feature-activation.ts's shared resolver precedence (mirroring #4099's approach), while the

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 509 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 8, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Closing unmerged — see #4101's closing comment for the full evidence. This PR's own gittensory-orb review correctly flagged that it changes no runtime behavior (the required-AND it "fixes" was already equivalent to the kill-switch+decide shape, since reviewMemory never had an allowlist term to bypass, unlike inlineComments/fixHandoff in #4116). An independent adversarial re-verification confirmed the bot's blocker was correct, not a false positive. Shipping this as-is would overclaim a fix that isn't there.

@JSONbored JSONbored closed this Jul 8, 2026
@JSONbored
JSONbored deleted the fix/4101-config-as-code-kill-switch branch July 8, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate + close the config-as-code gap for reviewMemory

1 participant