scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-08-28 05:25:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

docs(group): remove REVIEW-FINDINGS-MAP.md

Browse source 
Removes the findings-to-commits ledger from the source tree.

Note for anyone reading this in history: the file was introduced on main by
#3012 and carried that PR's findings map; this branch had appended a #3020
section. Deleting it drops both. #3012's content is recoverable with
`git show 2c0fb7753:gitnexus/src/core/group/REVIEW-FINDINGS-MAP.md`.
This commit is contained in:
Gergo Magyar 2026-08-27 15:11:26 +00:00
parent cf8d5f8d72
commit 8f7c25dd3e
1 changed files with 0 additions and 157 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

157
gitnexus/src/core/group/REVIEW-FINDINGS-MAP.md
View file

@ -1,157 +0,0 @@
# Review findings → commits
One section per reviewed PR, newest last.
## PR #3012
Every finding raised in review of this PR, and the commit that closes it. The
Definition of Done claims each finding has exactly one commit and that reverting
that commit reintroduces that finding and no other; this is what makes the claim
checkable without the reviewer's report in hand.
**Not under `docs/`** — that path is gitignored, so a map written there would
never reach the PR and nobody but its author could perform the audit. It lives
beside the code it describes, as `PIPELINE.md` does.
## Revert contract
Revertability is **dependency-aware**. Where one commit extracts a helper that
later commits consume, reverting the helper alone does not build. The contract
is: reverting a commit reintroduces its own finding and no other _finding_, with
its prerequisite commits retained.
One coupled set exists:
| Set | Commits | Why coupled |
| -------------------------- | --------------------------------------------------- | -------------------------------------------------------------------------------------- |
| Shared completeness helper | `4c203ac7b` ← `79f6f5bcb`, `0fe6fc9d4`, `dbc3953b0` | The three consumers call `crossRepoCompleteness`; reverting it alone breaks the build. |
## Primary findings
| # | Finding | Commit |
| --- | -------------------------------------------------------------------------------- | ----------- |
| 1 | Malformed `meta.json` crashes cross-repo impact and leaks the bridge handle | `27b0069f2` |
| 2 | Unreadable repos still contribute contracts through deferred manifest resolution | `7037e8441` |
| 3 | Strict read accepts a registry row that cannot identify a repo | `5245b22d7` |
| 4 | Unstamped bridge metadata is trusted without any check | `94f2a8757` |
| 5 | A subgroup-scoped query is marked incomplete by repos it excluded | `79f6f5bcb` |
| 6 | The preserved registry and the bridge disagree about the same sync | `4676abf03` |
| 7 | Three surfaces compute completeness three different ways | `4c203ac7b` |
| 8 | `group_contracts` has no channel for its own completeness | `0fe6fc9d4` |
| 9 | `group status` cannot tell a missing entry from an unreadable registry | `a12b846c9` |
| 10 | The sync summary describes a write that did not happen that way | `5a668455c` |
| 11 | The total-failure log promises preservation where there is nothing to preserve | `c4b356b29` |
| 12 | The bridge-failure warning promises a truncation the code never reports | `1df79bb9a` |
| 13 | Two concurrent syncs of one group lose each other's writes | `4f07359bf` |
| 14 | The bridge swap needs the lock its caller already holds | `3b6215862` |
| 15 | The byte guard misses most tracked text files, and all extensionless ones | `07bf8be75` |
| 16 | The byte guard reads the vendored grammar tree it does not need to judge | `3ef831a0a` |
| 17 | The strict-read test cannot see which registry read ran | `eccc3c682` |
| 18 | The CLI branches this PR introduced have no assertions | `535d2ad29` |
| 19 | The MCP payloads have no assertions | `2c253b4a8` |
| 20 | Corrupt-registry errors quote the file's bytes, credentials included | `24ba2a537` |
| 21 | The mtime pairing's limits are recorded nowhere a reader will look | `ca0aca106` |
| 22 | The bridge-input docstring narrows what `unreadableRepos` means | `8c930f470` |
| 23 | The strict-read docstring's call-site count is wrong | `a95838954` |
| 24 | Contract staging crashes on the engine's argument limit | `57eac7558` |
| 25 | The sync tool's description names two of three reachable outcomes | `8bfd1a6ab` |
| 26 | The impact tool and status resource do not explain incompleteness | `dbc3953b0` |
| 27 | A lock timeout blames an `analyze` it cannot establish | `2d2a0119e` |
| 28 | A losing sync downgrades the one that beat it to the lock | `e407f05cf` |
## Findings raised in review and deliberately not implemented as suggested
| Finding | Suggested fix | What shipped, and why |
| ---------------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| Unstamped metadata is trusted | Treat every absent stamp as incomplete | Rejected. It would mark every pre-existing bridge a lower bound until re-synced — a repo-wide regression traded for a narrow window. The write-order pairing in `94f2a8757` is the narrower fix. |
| Stale bridge signal after a failed write | Re-stamp the metadata so the warning's promise becomes true | Rejected. Re-stamping recreates the metadata/database mis-pairing that stamping exists to prevent. `1df79bb9a` corrects the warning instead. |
| Strict row gate | Require all three fields non-blank | Narrowed to `name` and `storagePath`. This gate rejects the whole registry, which is machine-wide, so a field tightened past what identification needs lets one blank value break every group sync on the machine. |
## Found during execution, not in the review
| What | Commit |
| --------------------------------------------------------------------------------------------- | ----------- |
| A half-written bridge stamp read as a verified match (found by the repo's own contract check) | `066f2d802` |
| `readBridgeMeta`'s widened return type blocked the merge on contract drift | `a9d281dd4` |
| `group contracts --json` discarded every field it did not re-serialize | `b7753575d` |
| `sync.ts` renders as a binary diff because the base blob carries a NUL | `1667c24b4` |
## Corrections to the plan, found while executing it
Recorded because each was a claim in the plan that the code contradicted.
| Claim | Reality |
| ------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| The strict gate should require the fields "the resolution path consumes" | `defaultResolveHandle` **does** consume `path`. The distinction is what _identifies_ the repo. |
| Pass the trace's two endpoint repos as the scope predicate | A destination trace declares no `to`. Narrowing to `from` would report an unreadable provider as "no outgoing link". |
| Filter the incomplete set by the subgroup prefix | The query's own repo must stay in scope, or an unreadable origin becomes a confident "nothing depends on this". |
| `group status`'s third failure mode is a row that resolves but cannot be opened | Unreachable — `loadMeta` returns `null` on every error and `checkStaleness` catches everything. The reachable case is `resolveRepo` throwing. |
| The mtime rule can only demote pairs already broken | False. `cp -r` and `rsync` without `-t` demote an intact pair. Recorded at the code in `ca0aca106`. |
| `.scm` files are "edited constantly" here | Every tracked `.scm` is vendored. This repo writes tree-sitter queries inline in TypeScript. |
## Residual risks, recorded rather than closed
- **Credentials in the registry.** HTTPS remote URLs are persisted with their
userinfo intact. `24ba2a537` stops one channel echoing them; it does not stop
them being written. Pre-existing, tracked separately.
- **`readRegistryFile`'s read error.** The ENOENT-guarded outer catch still
rethrows the raw `fs.readFile` error into `unresolvableReason`. Node embeds
the path, not file contents, so no registry bytes leak — but it is the one
remaining foreign error object on that path.
- **Abstract-socket lock scope.** Linux abstract sockets are
network-namespace-scoped, so two containers sharing a bind-mounted group
directory do not contend unless the file backend is forced. Recorded at
`group-lock.ts`.
- **Scope filter at depth > 1.** The declared-scope intersection is sound only
while `MAX_SUPPORTED_CROSS_DEPTH` is 1. At depth 2 an out-of-scope repo can
sit between two in-scope ones. Recorded at the intersection site.
- **R14 is unmet on this PR.** `.gitattributes` makes TypeScript diffs render as
text, and it works locally — but GitHub resolves the attribute from the base
side, which does not carry it. `sync.ts` renders as binary in this PR's web
view and will render as text for every PR after this one merges.
## PR #3020
Ten findings from the multi-engine review of this PR (five reviewer lenses plus
an independent cross-model pass; six independently validated).
### Revert contract
Same dependency-aware contract as above, with two departures stated plainly
rather than claimed away:
| Set | Commits | Why coupled |
| ------------------ | ----------------------------- | ----------------------------------------------------------------------------------------------- |
| Suppressed-stage marker | `2c8a266d5` ← `abda0d041` | The renderer and its CLI test read `suppressedMatchStages`; reverting the field alone does not build. |
`abda0d041` closes three findings (4, 5, 6), not one: they are the same code
block — a false comment, an ambiguous line, and the test that pins both.
Reverting it reintroduces all three. Splitting them would have produced a commit
whose test was red.
### Findings
| # | Finding | Commit |
| --- | ------------------------------------------------------------------------------ | ----------- |
| 1 | `matching.max_candidates_per_step` is dead config and the PR body claims it works | `9d05ea2de` |
| 2 | An exact-only sync persists a narrowed registry with no completeness marker | `2c8a266d5` |
| 3 | MCP `group_sync` accepts and silently ignores removed parameters, unlike the CLI | `fd21d2de7` |
| 4 | The `?? 0` fallback's comment describes a code path that cannot occur | `abda0d041` |
| 5 | The new per-stage `Matching:` report has no test coverage | `abda0d041` |
| 6 | `--exact-only` prints `wildcard: 0` for a stage that never ran | `abda0d041` |
| 7 | `detect.shared_libs` is the one DetectConfig key with no extractor | `9d05ea2de` |
| 8 | The string `"false"` suppresses wildcard links via `Boolean()` coercion | `fd21d2de7` |
| 9 | CLI `--exact-only` help omits that manifest links still emit | `fadf832ba` |
| 10 | `group_sync` tool description undersells non-HTTP contract extraction | `fadf832ba` |
### Not closed here
Finding 1 has two halves. The dead key is removed; the **PR description still
claims it is read and working** and must be corrected when the PR is updated —
that surface is outside this branch.
Deliberately deferred, with reasons, in the plan's Scope Boundaries: threading
the marker into `BridgeMeta` so `group_impact` and cross-repo `trace` stop
reporting completeness over a narrowed graph; a repo-wide unknown-parameter
layer for every MCP tool; folding completeness into `group status`.