Skip to content

Follow-up: deferred review findings from PR #817 #830

Description

@philcunliffe

Triage of PR #817 (integration/proxy-mode-default-attach, head ef99b06a) after its two review rounds found no blocking residue. Every finding below was re-verified against the tree at head, not taken from the review records. The design's central claim holds: LLP 0242 is implemented on master exactly as LLP 0251 and 0249 describe (manifest gateway_proxy_mode at hypaware-core/plugins-workspace/claude/hypaware.plugin.json:45, the composePickerConfig fold at src/core/cli/walkthrough.js:994/:1035, maybeOfferProxyModeMigration at src/core/commands/clients.js:821 with the non-interactive gate at :886, enableGatewayProxyMode at src/core/config/gateway_proxy_enable.js:69 with both refusals at :154), the gate suite is 45/45 at head, and the new --json pin goes cleanly red when either the :886 gate or the json propagation at :457 is perturbed. These deferred items remain:

1. PR #822 collision: LLP 0244 merge conflict plus LLP 0251 going stale (medium, merge-time action)

Open PR #822 (fix/issue-819) edits the same llp/0244-attach-migrates-to-proxy-mode.decision.md metadata block this PR touches. Trial-merging origin/fix/issue-819 into ef99b06a reproduces:

CONFLICT (content): Merge conflict in llp/0244-attach-migrates-to-proxy-mode.decision.md

(#817 appends , LLP 0251 to the Related: continuation line; #822 inserts an **Extended-by:** LLP 0259 block right after it. Keep both; nothing settled is touched. test/core/attach-proxy-migration.test.js auto-merges cleanly.)

Separately, once #822's LLP 0259 lands, two sentences in llp/0251-proxy-mode-default-attach.design.md section 3 become inaccurate: line 97 ("keyed on the config, not the CA") and lines 132-133 ("Once the key is set the offer never appears again"). LLP 0259 settles the opposite for the key-on / CA-missing case (its forward-ref on 0244: the gate reads the CA as well as the config, so proxy_mode: true with no CA reaches a repair). Today, with #822 unmerged, LLP 0251's prose is accurate against the tree, and pre-emptively citing LLP 0259 from this branch would mint a dangling citation, so this is correctly not fixed in #817.

Action for whichever of #817 / #822 merges second: resolve the 0244 metadata conflict by keeping both lines, and append **Extended-by:** LLP 0259 (#migration: the offer gate also reads the CA; key-on with no CA reaches a repair instead of returning) to LLP 0251's metadata.

No behavioural conflict: on the trial-merged tree the proxy suites run green and #822 preserves the parsed.json short-circuit verbatim.

2. Merge-order constraint: #815 first (informational, still true)

llp/0251 cites LLP 0245 (section 0 and section 3) and declares Depends-on: proxy-mode-capture, but llp/0245-* exists only on origin/integration/proxy-mode-capture (PR #815, still open; no other remote branch carries it, verified by sweep). Merging #817 first lands a design of record citing a document absent from the tree. The PR body's merge-notes block records this; nothing enforces it. Honor the ordering at merge time.

3. LLP 0249 line citations are commit-relative and have drifted at head (low)

llp/0249-proxy-mode-default-attach.plan.md cites walkthrough.js lines 973/1014/1276/1537, correct at its stated anchor 04330abb but a uniform +21 off at ef99b06a (real positions 994/1035/1297/1558). The doc names its anchor commit, so the citations are not wrong, merely stale when read against head. Same section's "32/32 across the five named test files" is 34/34 since T1 landed (45/45 with attach-enablement-state.test.js). LLP 0249 is Active; if anyone corrects it, do so as an editorial note rather than rewriting the audit record.


Backlink: PR #817. Review records: round 1 3efab8c2 and round 2 ef99b06a comments on that PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)neutral:stuckneutral attempted this but cannot complete it autonomously — needs a human

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions