Pin gate.ts and attribution.ts at 100% coverage - #78
Merged
Conversation
`security-critical-100` in the org testing rubric is reject severity: files on a security- or compliance-critical path carry a per-file 100% override above the global floor. Neither of these two did, and both were below it — gate.ts at 86.95% branches, attribution.ts at 85.71% functions. The global floor here is deliberately low (70/61/66/70) because the three alternate transports need the Agent SDK, a subprocess, or a live apiserver to exercise honestly. That makes an average an especially poor guardian for these two files: the package can sit comfortably above 70 with the whole gap inside the approval gate. gate.ts decides whether a factory PR ships — it parses each role's verdict, enforces the evidence contract by verifying that every cited fragment appears verbatim at the cited location, and blocks release on calibration drift. attribution.ts binds a session's cloud actions to a named human through STS SourceIdentity; a gap there is an action nobody is accountable for. ─── What the new tests cover ─── attribution: STS output that is not JSON at all, which must surface as a named error rather than a raw SyntaxError — an operator reads a different cause from that than from a well-formed response with no credentials. And each of the three partial credential sets, which are worse than an empty one because they would be exported into the environment and fail later, somewhere with no visible connection to attribution. gate: a citation with no file path, and one whose fragment is empty or whitespace — both must be refused before the search rather than matched by it, since an empty fragment trivially appears in any file. A reader that throws (EACCES, EISDIR) rather than returning null, which has to read as an unreadable tree rather than as fabrication, and must not escape and take down the gate run. A fragment longer than the whole cited file. An inline `quoted_fragment:` rather than a block scalar — the shape a model most often emits, and dropping it would strip the evidence off an otherwise well-formed citation, which then auto-downgrades the verdict for lacking evidence it did supply. Key/value lines before any list item. A block scalar of only blank lines. A grades block followed by another header. `applySelfReviewDowngrade` with nothing conflicted. And an unrecognized grade letter, which scores below F and so blocks release — worth pinning because the opposite reading is just as plausible to a reader and would let a garbled calibration ship silently. ─── Three unreachable guards ─── Reaching 100% surfaced three checks that cannot be false while the code around them stands. None is deleted to make a number go up — this is an approval gate, and trading a redundant guard for coverage is the wrong direction. `letterLevel`'s N/A arm and `parseQualityGrades`'s VALID_GRADES membership test are marked `v8 ignore` with the reason recorded: the first is unreachable because the sole caller filters N/A first, the second because the regex above it already enumerates the valid grades — and that redundancy is what stops a widened regex from admitting a grade the calibration path cannot score. Same for `defaultRunner` in attribution.ts, which binds execFile with a timeout and holds no logic; covering it would mean shelling out to the real `aws` from a unit test. Two were better fixed than ignored. `containsRun`'s empty-needle check is redundant with the caller's guard, so it is gone and the caller's guarantee is stated. The `else if (key === 'quoted_fragment')` chain is now a plain `else`: the regex admits exactly four keys, so re-testing the last one added a branch that could not be false — and a fifth key would now silently do nothing rather than land there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
security-critical-100in the org testing rubric is reject severity: files on a security- or compliance-critical path carry a per-file 100% override above the global floor. Neither of these two did, and both were below it —gate.tsat 86.95% branches,attribution.tsat 85.71% functions.The global floor here is deliberately low (70/61/66/70) because the three alternate transports need the Agent SDK, a subprocess, or a live apiserver to exercise honestly. That makes an average an especially poor guardian for these two: the package can sit comfortably above 70 with the whole gap inside the approval gate.
gate.tsdecides whether a factory PR ships — it parses each role's verdict, enforces the evidence contract by verifying every cited fragment appears verbatim at the cited location, and blocks release on calibration drift.attribution.tsbinds a session's cloud actions to a named human through STS SourceIdentity; a gap there is an action nobody is accountable for.What the new tests cover
attribution — STS output that isn't JSON at all, which must surface as a named error rather than a raw
SyntaxError(an operator reads a different cause from that than from a well-formed response with no credentials). And each of the three partial credential sets, which are worse than an empty one because they'd be exported into the environment and fail later, somewhere with no visible connection to attribution.gate — a citation with no file path, and one whose fragment is empty or whitespace; both must be refused before the search rather than matched by it, since an empty fragment trivially appears in any file. A reader that throws (EACCES, EISDIR) rather than returning null, which has to read as an unreadable tree rather than fabrication and must not escape and take down the gate run. A fragment longer than the whole cited file. An inline
quoted_fragment:rather than a block scalar — the shape a model most often emits, and dropping it would strip evidence off an otherwise well-formed citation, which then auto-downgrades the verdict for lacking evidence it did supply. Key/value lines before any list item. A block scalar of only blank lines. A grades block followed by another header.applySelfReviewDowngradewith nothing conflicted. And an unrecognized grade letter, which scores below F and so blocks release — worth pinning because the opposite reading is just as plausible to a reader and would let a garbled calibration ship silently.Three unreachable guards
Reaching 100% surfaced three checks that can't be false while the code around them stands. None is deleted to make a number go up — this is an approval gate, and trading a redundant guard for coverage is the wrong direction.
letterLevel's N/A arm andparseQualityGrades's VALID_GRADES membership test are markedv8 ignorewith reasons recorded: the first is unreachable because the sole caller filters N/A first, the second because the regex above it already enumerates the valid grades — and that redundancy is exactly what stops a widened regex from admitting a grade the calibration path can't score. Same fordefaultRunner, which bindsexecFilewith a timeout and holds no logic; covering it would mean shelling out to the realawsfrom a unit test.Two were better fixed than ignored.
containsRun's empty-needle check is redundant with the caller's guard, so it's gone and the caller's guarantee is stated instead. Theelse if (key === 'quoted_fragment')chain is now a plainelse: the regex admits exactly four keys, so re-testing the last one added a branch that could not be false — and a fifth key would now silently do nothing rather than land there.